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.

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

Add a config to control if authgear verify the signature of saml elements from SP #4812

Merged
merged 7 commits into from
Oct 14, 2024
Merged

Add a config to control if authgear verify the signature of saml elements from SP #4812

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f76528c
Add a config to control if signature of SP requests should be verified
tung2744 Oct 9, 2024
49073e4
Respect the new flag when checking saml signature
tung2744 Oct 9, 2024
6e798de
Update the saml spec for the new should_verify_signature flag
tung2744 Oct 9, 2024
d7d06d5
Fix saml service tests
tung2744 Oct 10, 2024
4ad2ccb
Rename should_verify_signature to signature_verification_enabled
tung2744 Oct 10, 2024
3a03dd4
Always panic with error
tung2744 Oct 10, 2024
10a5fd4
Rename ShouldVerifySignature to SignatureVerificationEnabled in tests
tung2744 Oct 10, 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
14 changes: 10 additions & 4 deletions pkg/lib/saml/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,10 +563,13 @@ func (s *Service) IssueLogoutResponse(
func (s *Service) VerifyEmbeddedSignature(
sp *config.SAMLServiceProviderConfig,
samlElementXML string) error {
if !sp.ShouldVerifySignature {
return nil
}
certs, ok := s.SAMLSpSigningMaterials.Resolve(sp)
if !ok || len(certs.Certificates) == 0 {
// Signing cert not configured, nothing to verify
return nil
// This should be prevented by config validation. Therefore it is a programming error.
panic("SP certificates not configured but signature verification is required")
Copy link
Collaborator

Choose a reason for hiding this comment

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

panic with error. I will study whether there is golangci-lint rule to address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just curious, what is the advantage of panic with error instead of string?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So that when you call recover(), you can get an error. Otherwise, you will get a string. With golang error chaining, recover() will give you an error that you can further call errors.As and errors.Is on the recovered error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updated that link to include this rationale.

}
certificateStore := &dsig.MemoryX509CertificateStore{
Roots: slice.Map(certs.Certificates, func(c config.X509Certificate) *x509.Certificate {
Expand Down Expand Up @@ -601,10 +604,13 @@ func (s *Service) VerifyExternalSignature(
sigAlg string,
relayState string,
signature string) error {
if !sp.ShouldVerifySignature {
return nil
}
certs, ok := s.SAMLSpSigningMaterials.Resolve(sp)
if !ok || len(certs.Certificates) == 0 {
// Signing cert not configured, nothing to verify
return nil
// This should be prevented by config validation. Therefore it is a programming error.
panic("SP certificates not configured but signature verification is required")
Copy link
Collaborator

Choose a reason for hiding this comment

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

panic with error

}

q := url.Values{}
Expand Down