Skip to content

Service Provider creation without entity descriptor in config file#35454

Merged
flyinghermit merged 12 commits intomasterfrom
sshah/fetch-sp-metada
Dec 12, 2023
Merged

Service Provider creation without entity descriptor in config file#35454
flyinghermit merged 12 commits intomasterfrom
sshah/fetch-sp-metada

Conversation

@flyinghermit
Copy link
Copy Markdown
Contributor

@flyinghermit flyinghermit commented Dec 7, 2023

  • Tries to fetch entity descriptor (aka SP metadata) from remote entity ID endpoint. Requires entity ID to be configured.
  • If entity descriptor is not available from remote entity ID endpoint, a default entity descriptor with entity ID, ACS URL and unspecified NameID format will be created. Requires entity ID and ACS URL to be configured.
  • New proto field ACSURL (acs_url) is added to accept Assertion Consumer Service URL.

see #34661, https://github.com/gravitational/teleport.e/issues/2692

Manually tested with service providers.

Works only with tctl, Web UI will be updated with another PR.

To test this PR, create a new service provider with the following spec:

kind: saml_idp_service_provider
version: v1
metadata:
  name: sp
spec:
  entity_id: https://example.com/saml/metadata
  acs_url: https://example.com/saml/acs

It should generate entity descriptor with the following XML format:

<EntityDescriptor xmlns="urn:oasis:names:tc:SAML:2.0:metadata" validUntil="2023-12-10T03:30:20.578Z" entityID="https://example.com/saml/metadata">
      <SPSSODescriptor xmlns="urn:oasis:names:tc:SAML:2.0:metadata" validUntil="2023-12-10T03:30:20.577825Z" protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol" AuthnRequestsSigned="false" WantAssertionsSigned="true">
        <NameIDFormat>urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified</NameIDFormat>
        <AssertionConsumerService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://example.com/saml/acs" index="1"></AssertionConsumerService>
        <AssertionConsumerService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact" Location="https://example.com/saml/acs" index="2"></AssertionConsumerService>
      </SPSSODescriptor>
</EntityDescriptor>

Changelog: Added guided SAML entity descriptor creation when entity descriptor XML is not yet available.

- fetch ed from remote entity ID endpoint
- if ed is not available, generate a default ed with entity ID and acs url
- fetch and generate func tests
- add logger and http client to SAMLIdPServiceProviderService
@flyinghermit flyinghermit requested review from mdwn and r0mant December 8, 2023 05:24
@flyinghermit flyinghermit marked this pull request as ready for review December 8, 2023 05:24
Copy link
Copy Markdown
Contributor

@bl-nero bl-nero left a comment

Choose a reason for hiding this comment

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

The code looks good, but I was unable to properly test it (my local Teleport suddenly stopped receiving features from Sales Center and I can't force it to enable SAML).

Comment thread lib/services/local/saml_idp_service_provider.go Outdated
@flyinghermit
Copy link
Copy Markdown
Contributor Author

Friendly ping @r0mant @mdwn

Comment thread lib/services/local/saml_idp_service_provider.go Outdated
- move http client init to NewSAMLIdPServiceProviderService
- catch eof error in validateSAMLIdPServiceProvider for verbosity
Comment thread lib/services/local/saml_idp_service_provider.go Outdated
Copy link
Copy Markdown
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm with a couple of suggestions

Comment thread lib/services/local/saml_idp_service_provider.go
Comment thread lib/services/local/saml_idp_service_provider.go Outdated
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
return trace.Wrap(trace.ReadError(resp.StatusCode, nil))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we read response body and include it in the error and/or log it for easier troubleshooting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. Unless the error is from reading invalid xml or metadata type, I doubt response body would add any meaningful information what is not already provided with StatusCode.

@flyinghermit flyinghermit added this pull request to the merge queue Dec 12, 2023
Merged via the queue into master with commit 502fa7a Dec 12, 2023
@flyinghermit flyinghermit deleted the sshah/fetch-sp-metada branch December 12, 2023 01:34
@public-teleport-github-review-bot
Copy link
Copy Markdown

@flyinghermit See the table below for backport results.

Branch Result
branch/v14 Failed

flyinghermit added a commit that referenced this pull request Dec 12, 2023
github-merge-queue Bot pushed a commit that referenced this pull request Dec 13, 2023
…ile (#35657)

* Backport #35454 to branch/v14

* run make fix-imports

* use trace.Badparameter struct and wrap it for better stack strace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants