New saml 20240430 - Not to merge but just for SAML feature branch testing #2862
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/187527694 The labels on this github issue will be updated when the story is started. |
peterhaochen47
force-pushed
the
new-saml-20240430
branch
3 times, most recently
from
2039f71
to
c8edd59
Compare
Co-authored-by: Peter Chen <[email protected]> Co-authored-by: Bruce Ricard <[email protected]> Co-authored-by: Danny Faught <[email protected]>
* Instead of calling fail(). We have a suspicion that there is a bug in the way the tests are running (most of them are somehow not running with "./gradlew test" and we have a theory that a combination of mixing junit4 imports and the junit5 fail() might be contributing. * I was careful to use @ignore for tests importing the junit4 @test, and @disabled for tests using the junit5 @test. * These annotations were added, with the idea that you can search for '@ignore("SAML' and '@disabled("SAML' to find the tests that need attention before we finish the SAML library conversion. @ignore("SAML test fails") @ignore("SAML test doesn't compile") @ignore("SAML test setup doesn't compile") @disabled("SAML test fails") @disabled("SAML test doesn't compile") * A few tests are set to ignore because they're failing for the right reasons, but more work is needed to finish that and get back to green. The goal is to start tracking these annotations instead of failing tests, so we can stay green. * Tests now running: server module: 3,435 (in IntelliJ) (98 total ignored) uaa module: 67 (command line run of "./gradlew test" for all tests - still needs troubleshooting) Co-authored-by: Danny Faught <[email protected]>
Co-authored-by: Hongchol Sinn <[email protected]>
* Removed commented-out references to the outdated SAML extension library Co-authored-by: Duane May <[email protected]>
- Adds back endpoint and incorporates forwarding for new pattern saml2 endpoints, Still has some wip elements WithHttpsNotRequired > samlMetadataReturnsOk still red RelyingPartyRegistration is hardcoded in xml, /saml/metadata/ with trailing slash not working missing parity with develop [#186986697] Co-authored-by: Peter Chen <[email protected]>
…NotRequired -> samlMetadataReturnsOk is green
- fixed one test but still WithHttpsRequired > samlMetadataReturnsOk is red after fixing this test -
HealthzShouldNotBeProtectedMockMvcTests > WithHttpsRequired > samlMetadataRedirects() FAILED
java.lang.AssertionError: Range for response status value 200 expected:<REDIRECTION> but was:<SUCCESSFUL>
[#186986697]
Co-authored-by: Duane May <[email protected]>
Co-authored-by: Peter Chen <[email protected]> Co-authored-by: Bruce Ricard <[email protected]> Co-authored-by: Danny Faught <[email protected]>
* Instead of calling fail(). We have a suspicion that there is a bug in the way the tests are running (most of them are somehow not running with "./gradlew test" and we have a theory that a combination of mixing junit4 imports and the junit5 fail() might be contributing. * I was careful to use @ignore for tests importing the junit4 @test, and @disabled for tests using the junit5 @test. * These annotations were added, with the idea that you can search for '@ignore("SAML' and '@disabled("SAML' to find the tests that need attention before we finish the SAML library conversion. @ignore("SAML test fails") @ignore("SAML test doesn't compile") @ignore("SAML test setup doesn't compile") @disabled("SAML test fails") @disabled("SAML test doesn't compile") * A few tests are set to ignore because they're failing for the right reasons, but more work is needed to finish that and get back to green. The goal is to start tracking these annotations instead of failing tests, so we can stay green. * Tests now running: server module: 3,435 (in IntelliJ) (98 total ignored) uaa module: 67 (command line run of "./gradlew test" for all tests - still needs troubleshooting) Co-authored-by: Danny Faught <[email protected]>
- Adds back endpoint and incorporates forwarding for new pattern saml2 endpoints, Still has some wip elements WithHttpsNotRequired > samlMetadataReturnsOk still red RelyingPartyRegistration is hardcoded in xml, /saml/metadata/ with trailing slash not working missing parity with develop [#186986697] Co-authored-by: Peter Chen <[email protected]>
…NotRequired -> samlMetadataReturnsOk is green
- fixed one test but still WithHttpsRequired > samlMetadataReturnsOk is red after fixing this test -
HealthzShouldNotBeProtectedMockMvcTests > WithHttpsRequired > samlMetadataRedirects() FAILED
java.lang.AssertionError: Range for response status value 200 expected:<REDIRECTION> but was:<SUCCESSFUL>
[#186986697]
Co-authored-by: Peter Chen <[email protected]>
- With the new SAML lib, SAML SP metadata generation relies on a relyingPartyRegistration, which requires a valid SAML IDP metadata. In the context of UAA external SAML IDP login, UAA does not know what the SAML IDP metadata is, until the operator adds it via the /identity-providers endpoint. Also, some SAML IDPs might require you to supply the SAML SP metadata first before you can obtain the SAML IDP metadata. See relevant issue: spring-projects/spring-security#11369 - Previously, to solve this problem, the SAML SP metadata generation relies on relyingPartyRegistration values in saml-providers.xml, which hardcodes a SAML IDP metadata URL (point to some example Okta SAML instance); this means that UAA's SP metadata generation relies on the example Okta SAML instance to be running. - This commit, instead, supplies a hardcoded dummy SAML IDP metadata here to unblock the SAML SP metadata generation, at the advice of Spring Security team, so that UAA's functioning does not rely on some external running Okta instance. - code reference: https://github.com/spring-projects/spring-security-samples/blob/1b28351693d60f01a511cbcc18b64590452a3851/servlet/java-configuration/saml2/login/src/main/java/example/SecurityConfiguration.java#L62 [#186986697] Co-authored-by: Peter Chen <[email protected]>
- A continuation of 65d1f0f - This test is failing as early as e7beec7 due to the removal of SAML code, as this test is related the SAML feature [#186986697] Co-authored-by: Peter Chen <[email protected]>
* Has to be commented out of the erb file even when the test method used @disabled. Co-authored-by: Peter Chen <[email protected]>
- A continuation of 65d1f0f - This is a test recently added to develop branch, so ignoring this here because the SAML feature is still being built. [#186986697] Co-authored-by: Peter Chen <[email protected]>
- to reflect the fact that this IDP metadata just needs to exist in its bare minimal form, where the specific fields in it do not affect the SP metadata generation [#186986697] Co-authored-by: Peter Chen <[email protected]>
- previously some tests error with: ``` net.shibboleth.utilities.java.support.xml.XMLParserException: Unable to parse inputstream, it contained invalid XML ``` - this issue is fixed once we switch to loading the idp saml metadata via a file (instead of an InputStream) [186822654] Co-authored-by: Danny Faught <[email protected]>
Co-authored-by: Danny Faught <[email protected]>
* We're reprioritizing the test to get this test to pass. Co-authored-by: Bruce Ricard <[email protected]>
Co-authored-by: Duane May <[email protected]>
Co-authored-by: Duane May <[email protected]>
…ect request - Tests are failing but they are behaving as expected with curl and browser for /saml/metadata /saml/metadata/example and /saml/metadata/example/ - /saml/metadata/ is not returning xml - The dispatcher ordering along with position in the filter-mapping must be set properly. [#186986697] Co-authored-by: Bruce Ricard <[email protected]>
Co-authored-by: Duane May <[email protected]>
…VC Tests - /saml/metadata/ is not returning xml [#186986697] Co-authored-by: Filip Hanik <[email protected]>
Co-authored-by: Alicia Yingling <[email protected]> Co-authored-by: Duane May <[email protected]>
- to be processed by a single class "SamlConfiguration" where the @ConfigurationProperties(prefix="login.saml") annotation has the ability to process all fields under the login.saml section of UAA.yml - this is helpful because we can now centrally read, process, even validate all saml config fields under "login.saml" - pay attention to @ConfigurationProperties annotation's various requirements though: such as the private field names need to match the actually UAA.yml field name (e.g.: login.saml.fooBar -> private String fooBar); and that there need to be public setters and getters for each field - see: https://docs.spring.io/spring-boot/docs/current/reference/html/features.html#features.external-config.typesafe-configuration-properties.using-annotated-types - the exception of the saml entity id, which in UAA.yml is somehow outside of the login.saml context (set by login.entityID) so that field stays under class SamlEntityIdConfiguration Co-authored-by: Duane May <[email protected]>
- these getters and setters are required for @ConfigurationProperties annotation to work; use lombok so that we don't need to explicitly define them [186822654] Co-authored-by: Duane May <[email protected]>
- as @DaTa covers the getters and setters Co-authored-by: Duane May <[email protected]>
- configure the file name of the saml sp metadata (the downloaded xml file name when accessing the metadata endpoint: http://localhost:8080/uaa/saml/metadata) to match the status quo on develop branch: "saml-sp.xml" - This file name likely do not matter, but out of caution, we should maintain the same file name as before [186822654] Co-authored-by: Duane May <[email protected]>
- now that the metadata is being provided at the correct location: /saml/metadata, we can correct the test expectation to reflect that (hence matching the develop branch) [#186986697] Co-authored-by: Duane May <[email protected]>
- Removed forwarding of `/saml/metadata` endpoint to `/saml/metadata/example`. It is not necessary because `/saml/metadata` endpoint method already calls `/saml/metadata/{registrationId}` with `example` as the default registrationId. (See class `SamlMetadataEndpoint`.)
- Made `HttpsEnforcementFilter` to be added to the top of the `SecurityFilterChainPostProcessor`'s `SecurityFilterChain`.
- Added `secFilterOpen06SAMLMetadata` to `SecurityFilterChainPostProcessor`'s `redirectToHttps` list.
[#186986697]
Co-authored-by: Duane May <[email protected]>
Co-authored-by: Peter Chen <[email protected]>
- Removed SamlExtensionUrlForwardingFilter. Just commented out for now in case we need it later. - Removed unneeded comments in test code. [#186986697] Co-authored-by: Duane May <[email protected]>
[#187084275] Co-authored-by: Duane May <[email protected]>
- Change SamlRelyingPartyRegistrationRepository to Configuration - Use constructor args instead of Autowired Co-authored-by: Duane May <[email protected]>
still had opensaml 3.4.6 Co-authored-by: Duane May <[email protected]>
- when SAML IDP is configured via uaa.yml, when
the user goes to "/uaa/saml2/authenticate/{saml-idp-alias}",
they will get sent to the configured SAML IDP with
a SAML authn request. Specifically, spring-security will do
the following:
- when the IDP's Binding mode is "HTTP-Redirect", the
user is redirected to the IDP
- when the IDP's Binding mode is "HTTP-POST", the user's
browser is triggered to POST to the IDP. For this to work,
the ContentSecurityPolicyFilter needs to updated to exempt
"/saml2" from policy enforcement, such that the script that
initiates the POST can be executed in the browser. Similar
to how this filter exempts /saml (the existing saml-related
path on develop branch).
- refactor: update the dummy IDP metadata file
dummy-saml-idp-metadata.xml to not point
to example.com, but to https://www.cloudfoundry.org
(which is more of a known destination)
- refactor: use constant DEFAULT_REGISTRATION_ID
[#187084275]
Co-authored-by: Duane May <[email protected]>
peterhaochen47
force-pushed
the
new-saml-20240430
branch
from
c8edd59
to
6249e49
Compare
When refreshing a token, always rotate for public clients, thus not requiring rotation to be enabled for all clients and preventing the possible error condition for public clients. Change-Id: I6ab80dd8b1928ab55863cea52849ff22f35c2779
Bumps [nokogiri](https://github.com/sparklemotion/nokogiri) from 1.16.4 to 1.16.5. - [Release notes](https://github.com/sparklemotion/nokogiri/releases) - [Changelog](https://github.com/sparklemotion/nokogiri/blob/main/CHANGELOG.md) - [Commits](sparklemotion/nokogiri@v1.16.4...v1.16.5) --- updated-dependencies: - dependency-name: nokogiri dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [k8s.io/client-go](https://github.com/kubernetes/client-go) from 0.30.0 to 0.30.1. - [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md) - [Commits](kubernetes/client-go@v0.30.0...v0.30.1) --- updated-dependencies: - dependency-name: k8s.io/client-go dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…2892) Bumps [com.nimbusds:nimbus-jose-jwt](https://bitbucket.org/connect2id/nimbus-jose-jwt) from 9.39 to 9.39.1. - [Changelog](https://bitbucket.org/connect2id/nimbus-jose-jwt/src/master/CHANGELOG.txt) - [Commits](https://bitbucket.org/connect2id/nimbus-jose-jwt/branches/compare/9.39.1..9.39) --- updated-dependencies: - dependency-name: com.nimbusds:nimbus-jose-jwt dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* fix: /info docs test expectation
- the value of "idpDefinitions.*" of /info response is in reality
a String, not an array. Here is how it looks like:
```
"idpDefinitions": {
"testsaml-redirect-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-redirect-binding&isPassive=true",
"testsaml-post-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-post-binding&isPassive=true"
},
```
which is consistent with the description of "idpDefinitions" field: "A
list of alias/url pairs of SAML IDP providers configured. Each url is
the starting point to initiate the authentication process for the SAML
identity provider."
- previously, this test is passing only because the test uaa.yml used
in this test contains no IDP configs; but once you input some valid IDP
configs, this test would fail since the value is actually a String.
- also clarify the description text
Additional commit squashed:
* change to VARIES
see https://docs.spring.io/spring-restdocs/docs/current/api/org/springframework/restdocs/payload/JsonFieldType.html
-> A variety of different types.
---------
Co-authored-by: d036670 <[email protected]>
prefix="login.saml" was in 2 ConfigProps classes before merged into 1
Reads provider info from database Passes the registrationId as relayState Signed-off-by: Prateek Gangwal <[email protected]>
when running multiple IT tests, the simplesamlphp2 link was also listed, and causing a conflict with url matcher Signed-off-by: Duane May <[email protected]>
|
Closing this as new PR replaces it #2908 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.