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

Show file tree
Hide file tree
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
4 changes: 3 additions & 1 deletion pkg/lib/config/saml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ var _ = Schema.Add("SAMLServiceProviderConfig", `
"assertion_valid_duration": { "$ref": "#/$defs/DurationString" },
"slo_enabled": { "type": "boolean" },
"slo_callback_url": { "type": "string", "format": "uri" },
"slo_binding": { "$ref": "#/$defs/SAMLSLOBinding" }
"slo_binding": { "$ref": "#/$defs/SAMLSLOBinding" },
"should_verify_signature": { "type": "boolean" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename this to signature_verification_enabled

},
"required": ["client_id", "acs_urls"],
"allOf": [
Expand Down Expand Up @@ -129,6 +130,7 @@ type SAMLServiceProviderConfig struct {
SLOEnabled bool `json:"slo_enabled,omitempty"`
SLOCallbackURL string `json:"slo_callback_url,omitempty"`
SLOBinding samlprotocol.SAMLBinding `json:"slo_binding,omitempty"`
ShouldVerifySignature bool `json:"should_verify_signature,omitempty"`
}

func (c *SAMLServiceProviderConfig) SetDefaults() {
Expand Down
14 changes: 14 additions & 0 deletions pkg/lib/config/secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,15 @@ func (c *SecretConfig) validateSAMLSigningKey(ctx *validation.Context, keyID str
ctx.EmitErrorMessage(fmt.Sprintf("saml idp signing key '%s' does not exist", keyID))
}

func (c *SecretConfig) validateSAMLServiceProviderCerts(ctx *validation.Context, sp *SAMLServiceProviderConfig) {
_, data, _ := c.LookupDataWithIndex(SAMLSpSigningMaterialsKey)
signingMaterials, _ := data.(*SAMLSpSigningMaterials)
certs, ok := signingMaterials.Resolve(sp)
if !ok || len(certs.Certificates) < 1 {
ctx.EmitErrorMessage(fmt.Sprintf("certificates of saml sp '%s' is not configured", sp.GetID()))
}
}

func (c *SecretConfig) Validate(appConfig *AppConfig) error {
ctx := &validation.Context{}

Expand Down Expand Up @@ -290,6 +299,11 @@ func (c *SecretConfig) Validate(appConfig *AppConfig) error {

if len(appConfig.SAML.ServiceProviders) > 0 {
c.validateRequire(ctx, SAMLIdpSigningMaterialsKey, "saml idp signing key materials")
for _, sp := range appConfig.SAML.ServiceProviders {
if sp.ShouldVerifySignature {
c.validateSAMLServiceProviderCerts(ctx, sp)
}
}
}
if appConfig.SAML.Signing.KeyID != "" {
c.validateSAMLSigningKey(ctx, appConfig.SAML.Signing.KeyID)
Expand Down
1 change: 1 addition & 0 deletions pkg/lib/config/testdata/config_tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1201,6 +1201,7 @@ config:
slo_enabled: true
slo_callback_url: https://authgear.cloud/slo
slo_binding: urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect
should_verify_signature: true
---
name: saml-signing
error: null
Expand Down