Add a config to control if authgear verify the signature of saml elements from SP #4812
Conversation
|
Test failed. |
pkg/lib/config/saml.go
Outdated
| @@ -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" } | |||
There was a problem hiding this comment.
Rename this to signature_verification_enabled
pkg/lib/saml/service.go
Outdated
| // 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") |
There was a problem hiding this comment.
panic with error. I will study whether there is golangci-lint rule to address this.
There was a problem hiding this comment.
Just curious, what is the advantage of panic with error instead of string?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Updated that link to include this rationale.
pkg/lib/saml/service.go
Outdated
| // 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") |
There was a problem hiding this comment.
|
Updated, thanks! |
|
Build failed. |
|
Fixed, thanks! |
ref DEV-2145