Skip to content
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.

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

backfill tests: SAML SP metadata #2794

Merged
merged 4 commits into from
Apr 1, 2024

Conversation

peterhaochen47
Copy link
Member

  • in preparation for replacing the EOL spring saml extension lib with spring security core saml, adding more test coverage on the SAML SP metadata
  • tests that SAML SP metadata matches the UAA configs (in the context of this test, the UAA configs are from the local uaa.yml used to start a local server)
  • also explicitly declare some SAML-SP-related fields in the said local uaa.yml to make the inputs to the test clearer

[#186986697]

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187289884

The labels on this github issue will be updated when the story is started.

@peterhaochen47 peterhaochen47 changed the title [WIP] backfill tests: SAML SP metadata [WIP - no review needed] backfill tests: SAML SP metadata Mar 21, 2024
@peterhaochen47 peterhaochen47 changed the title [WIP - no review needed] backfill tests: SAML SP metadata [WIP - do not need review for now] backfill tests: SAML SP metadata Mar 21, 2024
- in preparation for replacing the EOL spring saml extension lib
with spring security core saml, adding more test coverage on
the SAML SP metadata
- tests that SAML SP metadata matches the UAA configs (in the
context of this test, the UAA configs are from the local uaa.yml
used to start a local server)
- also explicitly declare some SAML-SP-related fields in the said
local uaa.yml to make the inputs to the test clearer

[#186986697]
@peterhaochen47 peterhaochen47 force-pushed the pr/develop/backfill-saml-sp-metadata-e2e-tests branch from 5bee567 to 88ecdd8 Compare March 21, 2024 23:48
hsinn0 and others added 2 commits March 27, 2024 15:40
- Replaced the code that was depending on the platform where the test was being executed.

[#186986697]

Co-authored-by: Danny Faught <danny.faught@broadcom.com>
Co-authored-by: Hongchol Sinn <hongchol.sinn@broadcom.com>
@swalchemist swalchemist changed the title [WIP - do not need review for now] backfill tests: SAML SP metadata backfill tests: SAML SP metadata Mar 27, 2024
@swalchemist swalchemist marked this pull request as ready for review March 27, 2024 23:33
@swalchemist swalchemist requested a review from a team March 27, 2024 23:33
@swalchemist swalchemist marked this pull request as draft March 28, 2024 16:32
* Looks like we used the wrong metadata when we added this assertion.
@swalchemist swalchemist marked this pull request as ready for review March 28, 2024 17:32
Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

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

@hsinn0
Copy link
Contributor

hsinn0 commented Mar 28, 2024

wouldn't it make sense to use default SHA256 also here https://github.com/cloudfoundry/uaa/blob/develop/uaa/src/main/webapp/WEB-INF/spring/saml-providers.xml#L305

Right. It was mentioned and considered before in a different PR. In production though, for TAS at least, the property is always set to either 256 or 512, and we decided just to leave it like that as it is practically only used for dev build.

@strehle
Copy link
Member

strehle commented Mar 28, 2024

wouldn't it make sense to use default SHA256 also here https://github.com/cloudfoundry/uaa/blob/develop/uaa/src/main/webapp/WEB-INF/spring/saml-providers.xml#L305

Right. It was mentioned and considered before in a different PR. In production though, for TAS at least, the property is always set to either 256 or 512, and we decided just to leave it like that as it is practically only used for dev build.

even in DEV it is no 256 as default... but therefore I would not let in xml sha1 with a settings where is looks like this is the default, e.g. https://github.com/cloudfoundry/uaa/blob/develop/uaa/src/main/webapp/WEB-INF/spring/saml-providers.xml#L305 because the value is no not null thus default in XML is never used

@peterhaochen47
Copy link
Member Author

peterhaochen47 commented Mar 28, 2024

wouldn't it make sense to use default SHA256 also here https://github.com/cloudfoundry/uaa/blob/develop/uaa/src/main/webapp/WEB-INF/spring/saml-providers.xml#L305

Right. It was mentioned and considered before in a different PR. In production though, for TAS at least, the property is always set to either 256 or 512, and we decided just to leave it like that as it is practically only used for dev build.

In cf-deployment (the OSS version of CF), this value is unset, hence UAA default will be used.

@strehle
Copy link
Member

strehle commented Mar 28, 2024

(the OSS version of CF), this value is unset, hence UAA default will be used.

I bet that even in cf-deployment now SHA256 is used... but I have no problem with it... only because of misleading XML default which is no default anymore..

See
curl -k https://login.uaa-acceptance.cf-app.com/saml/metadata |grep sha

Then merge this PR and check again the signature of https://login.uaa-acceptance.cf-app.com

@hsinn0
Copy link
Contributor

hsinn0 commented Mar 28, 2024

wouldn't it make sense to use default SHA256 also here https://github.com/cloudfoundry/uaa/blob/develop/uaa/src/main/webapp/WEB-INF/spring/saml-providers.xml#L305

Right. It was mentioned and considered before in a different PR. In production though, for TAS at least, the property is always set to either 256 or 512, and we decided just to leave it like that as it is practically only used for dev build.

In cf-deployment (the OSS version of CF), this value is unset, hence UAA default will be used.

Right, but the default has been always SHA1 there, as the property has been like that all the time. Do you think we should change the default value there for the OSS version of CF? Then we can make the change in another PR.

@strehle
Copy link
Member

strehle commented Mar 28, 2024

default value there for the OSS version of CF? Then we can make the change in another PR

ok

hsinn0 added a commit that referenced this pull request Mar 28, 2024
- For the bean.
- As suggested in review comments in #2794.
hsinn0 added a commit that referenced this pull request Apr 1, 2024
- For the bean.
- As suggested in review comments in #2794.
@hsinn0 hsinn0 merged commit 02c956e into develop Apr 1, 2024
20 checks passed
@hsinn0 hsinn0 deleted the pr/develop/backfill-saml-sp-metadata-e2e-tests branch April 1, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants