Skip to content

fix: fetchAndSetEntityDescriptor: return non-nil error if http status is not 200 #38825

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
flyinghermit merged 2 commits into from
Mar 5, 2024
Merged

fix: fetchAndSetEntityDescriptor: return non-nil error if http status is not 200 #38825

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
b55e20e
fix: : return non-nil error regardless of http status
flyinghermit Feb 29, 2024
ee1b015
use trace.Badparameter, bring debug log back, remove defensive sp.Get…
flyinghermit Mar 5, 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
13 changes: 7 additions & 6 deletions lib/services/local/saml_idp_service_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,14 @@ func (s *SAMLIdPServiceProviderService) GetSAMLIdPServiceProvider(ctx context.Co
// CreateSAMLIdPServiceProvider creates a new SAML IdP service provider resource.
func (s *SAMLIdPServiceProviderService) CreateSAMLIdPServiceProvider(ctx context.Context, sp types.SAMLIdPServiceProvider) error {
if sp.GetEntityDescriptor() == "" {
// fetchAndSetEntityDescriptor is expected to return error if it fails
// to fetch a valid entity descriptor.
if err := s.fetchAndSetEntityDescriptor(sp); err != nil {
s.log.Debugf("Failed to fetch entity descriptor from %q. %v.", sp.GetEntityID(), err)
// We aren't interested in checking error type as any occurrence of error mean entity descriptor was not set.
// But a debug log should be helpful to indicate source of error.
s.log.Debugf("Failed to fetch entity descriptor from %s. %s.", sp.GetEntityID(), err.Error())
Comment thread
flyinghermit marked this conversation as resolved.

if err := s.generateAndSetEntityDescriptor(sp); err != nil {
return trace.BadParameter("could not generate entity descriptor with given entity_id and acs_url.")
return trace.BadParameter("could not generate entity descriptor with given entity_id %q and acs_url %q: %v",
sp.GetEntityID(), sp.GetACSURL(), err)
}
}
}
Expand Down Expand Up @@ -256,7 +257,7 @@ func (s *SAMLIdPServiceProviderService) fetchAndSetEntityDescriptor(sp types.SAM
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
return trace.Wrap(trace.ReadError(resp.StatusCode, nil))
return trace.Wrap(trace.BadParameter("unexpected response status: %q", resp.StatusCode))
}

body, err := utils.ReadAtMost(resp.Body, teleport.MaxHTTPResponseSize)
Expand All @@ -277,7 +278,7 @@ func (s *SAMLIdPServiceProviderService) fetchAndSetEntityDescriptor(sp types.SAM
// generateAndSetEntityDescriptor generates and sets Service Provider entity descriptor
// with ACS URL, Entity ID and unspecified NameID format.
func (s *SAMLIdPServiceProviderService) generateAndSetEntityDescriptor(sp types.SAMLIdPServiceProvider) error {
s.log.Infof("Generating a default entity_descriptor with entity_id %s and acs_url %s.", sp.GetEntityID(), sp.GetACSURL())
s.log.Infof("Generating a default entity_descriptor with entity_id %q and acs_url %q.", sp.GetEntityID(), sp.GetACSURL())

acsURL, err := url.Parse(sp.GetACSURL())
if err != nil {
Expand Down
7 changes: 7 additions & 0 deletions lib/services/local/saml_idp_service_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,8 @@ func TestCreateSAMLIdPServiceProvider_fetchAndSetEntityDescriptor(t *testing.T)
switch r.RequestURI {
case "/status-not-ok":
w.WriteHeader(http.StatusNotFound)
case "/status-302-found":
w.WriteHeader(http.StatusFound)
case "/invalid-metadata":
fmt.Fprintln(w, "test")
default:
Expand All @@ -344,6 +346,11 @@ func TestCreateSAMLIdPServiceProvider_fetchAndSetEntityDescriptor(t *testing.T)
entityID: fmt.Sprintf("%s/status-not-ok", testSPServer.URL),
wantErr: true,
},
{
name: "status 302 found",
entityID: fmt.Sprintf("%s/status-302-found", testSPServer.URL),
wantErr: true,
},
{
name: "invalid metadata",
entityID: fmt.Sprintf("%s/invalid-metadata", testSPServer.URL),
Expand Down