Skip to content

Remove slo from saml metadata #10453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vrajmohan merged 2 commits into from
Apr 22, 2024
Merged

Remove slo from saml metadata #10453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2185f9c
Bump saml-idp version to 0.20.2-18f
vrajmohan Apr 16, 2024
bca4ee7
LG-7734: Remove SingleLogoutService from SAML metadata
vrajmohan Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ gem 'rqrcode'
gem 'ruby-progressbar'
gem 'ruby-saml'
gem 'safe_target_blank', '>= 1.0.2'
gem 'saml_idp', github: '18F/saml_idp', tag: '0.20.0-18f'
gem 'saml_idp', github: '18F/saml_idp', tag: '0.20.2-18f'
gem 'scrypt'
gem 'simple_form', '>= 5.0.2'
gem 'stringex', require: false
Expand Down
6 changes: 3 additions & 3 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ GIT

GIT
remote: https://github.com/18F/saml_idp.git
revision: f86b4c5ef4281a53b3f13a1db2c2e5839fdf077d
tag: 0.20.0-18f
revision: dd8643b16c8214f7b791763538180d043af7ef65
tag: 0.20.2-18f
specs:
saml_idp (0.19.3.pre.18f)
saml_idp (0.20.2.pre.18f)
activesupport
builder
faraday
Expand Down
7 changes: 0 additions & 7 deletions app/services/saml_endpoint.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,6 @@ def x509_certificate
def saml_metadata
config = SamlIdp.config.dup
config.single_service_post_location += year
if IdentityConfig.store.include_slo_in_saml_metadata
config.single_logout_service_post_location += year
config.remote_logout_service_post_location += year
else
config.single_logout_service_post_location = nil
config.remote_logout_service_post_location = nil
end

SamlIdp::MetadataBuilder.new(
config,
Expand Down
1 change: 0 additions & 1 deletion config/application.yml.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ in_person_outage_expected_update_date: 'October 31, 2024'
in_person_outage_emailed_by_date: 'November 1, 2024'
in_person_send_proofing_notifications_enabled: false
in_person_stop_expiring_enrollments: false
include_slo_in_saml_metadata: false
Copy link
Copy Markdown
Contributor

@Sgtpluck Sgtpluck Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[thought] i think we can't drop the config value until after the code change is deployed. 50-50 state deployment is affected by the config -- can you check to see if we need to keep this in here for deployment, and then have another PR to clean it up?

Copy link
Copy Markdown
Contributor

@aduth aduth Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% confident, but I think it'd be okay, since the old boxes would still have both the config value and code paths using the config?

Copy link
Copy Markdown
Contributor Author

@vrajmohan vrajmohan Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this.

Copy link
Copy Markdown
Contributor Author

@vrajmohan vrajmohan Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can't be 100% confident, I am OK with breaking it up into 2 deploys. Is there a way we can get to 100% confidence, perhaps by looping in other engineers? This also indicates the need to document the details of how production configuration works.

Copy link
Copy Markdown
Contributor Author

@vrajmohan vrajmohan Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Digging into this further, I agree with @aduth that the each instance would either have the config value and code that uses it, or neither. In theory, we should be good. What do you all think?

Copy link
Copy Markdown
Contributor

@Sgtpluck Sgtpluck Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know anything about how this works, so if you both are reasonably confident, i'm happy to trust you.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just hopping on to confirm, dropping a default config value in a PR is safe, because as previously stated, each box has its own in-memory copy of the config.

Removing the value from the S3 in the deployed env before this code is deployed: unsafe (because we may need to roll back, then the re-deployed old code would look for configs that aren't there)

invalid_gpo_confirmation_zipcode: '00001'
logins_per_ip_track_only_mode: false
# LexisNexis #####################################################
Expand Down
1 change: 0 additions & 1 deletion lib/identity_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,6 @@ def self.build_store(config_map)
config.add(:in_person_send_proofing_notifications_enabled, type: :boolean)
config.add(:in_person_state_id_controller_enabled, type: :boolean)
config.add(:in_person_stop_expiring_enrollments, type: :boolean)
config.add(:include_slo_in_saml_metadata, type: :boolean)
config.add(:invalid_gpo_confirmation_zipcode, type: :string)
config.add(:lexisnexis_account_id, type: :string)
config.add(:lexisnexis_base_url, type: :string)
Expand Down
31 changes: 0 additions & 31 deletions spec/features/saml/multiple_endpoints_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,36 +91,5 @@
['/api/saml/auth', endpoint_suffix].join(''),
)
end

it 'does not include logout urls if configured' do
allow(IdentityConfig.store).to receive(:include_slo_in_saml_metadata).
and_return(false)
document = REXML::Document.new(page.html)
logout_nodes = REXML::XPath.match(document, '//SingleLogoutService')
expect(logout_nodes.count).to be_zero
end

context 'when configured to include logout endpoints' do
before do
allow(IdentityConfig.store).to receive(:include_slo_in_saml_metadata).
and_return(true)
end

it 'includes the front-channel logout url' do
visit endpoint_metadata_path
document = REXML::Document.new(page.html)
logout_nodes = REXML::XPath.match(document, '//SingleLogoutService')
expect(logout_nodes.count { |n| n['Location'].match?(%r{/api/saml/logout\d{4}}) }).
to eq(2)
end

it 'includes the remote logout url' do
visit endpoint_metadata_path
document = REXML::Document.new(page.html)
logout_nodes = REXML::XPath.match(document, '//SingleLogoutService')
expect(logout_nodes.count { |n| n['Location'].match?(%r{/api/saml/remotelogout\d{4}}) }).
to eq(1)
end
end
end
end
22 changes: 0 additions & 22 deletions spec/services/saml_endpoint_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,27 +79,5 @@

expect(result.configurator.single_service_post_location).to match(%r{api/saml/auth2024\Z})
end

it 'does not include the SingLogoutService endpoints when configured' do
allow(IdentityConfig.store).to receive(:include_slo_in_saml_metadata).
and_return(false)
result = subject.saml_metadata

expect(result.configurator.single_logout_service_post_location).to be_nil
expect(result.configurator.remote_logout_service_post_location).to be_nil
end

it 'includes the SingLogoutService endpoints when configured' do
allow(IdentityConfig.store).to receive(:include_slo_in_saml_metadata).
and_return(true)
result = subject.saml_metadata

expect(result.configurator.single_logout_service_post_location).to match(
%r{api/saml/logout2024\Z},
)
expect(result.configurator.remote_logout_service_post_location).to match(
%r{api/saml/remotelogout2024\Z},
)
end
end
end