From 1f5f756fce983cf625aac164cfbe5038bae84e4e Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 11 Jan 2024 16:14:53 +0100 Subject: [PATCH] Make Wire options more robust --- acme/api/order.go | 9 +- acme/api/order_test.go | 2 +- acme/api/wire_integration_test.go | 2 +- acme/challenge.go | 13 +- authority/provisioner/options.go | 10 +- authority/provisioner/wire/dpop_options.go | 12 +- authority/provisioner/wire/oidc_options.go | 31 +++-- authority/provisioner/wire/wire_options.go | 39 ++++++ .../provisioner/wire/wire_options_test.go | 124 ++++++++++++++++++ 9 files changed, 220 insertions(+), 22 deletions(-) create mode 100644 authority/provisioner/wire/wire_options_test.go diff --git a/acme/api/order.go b/acme/api/order.go index 350b2825..bb7786b0 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -282,13 +282,16 @@ func newAuthorization(ctx context.Context, az *acme.Authorization) error { if err != nil { return acme.WrapError(acme.ErrorMalformedType, err, "failed parsing ClientID") } - + wireOptions, err := prov.GetOptions().GetWireOptions() + if err != nil { + return acme.WrapErrorISE(err, "failed getting Wire options") + } var targetProvider interface{ EvaluateTarget(string) (string, error) } switch typ { case acme.WIREOIDC01: - targetProvider = prov.GetOptions().GetWireOptions().GetOIDCOptions() + targetProvider = wireOptions.GetOIDCOptions() case acme.WIREDPOP01: - targetProvider = prov.GetOptions().GetWireOptions().GetDPOPOptions() + targetProvider = wireOptions.GetDPOPOptions() default: return acme.NewError(acme.ErrorMalformedType, "unsupported type %q", typ) } diff --git a/acme/api/order_test.go b/acme/api/order_test.go index 566ec53d..aaa356f9 100644 --- a/acme/api/order_test.go +++ b/acme/api/order_test.go @@ -1720,7 +1720,7 @@ func TestHandler_NewOrder(t *testing.T) { Wire: &wire.Options{ OIDC: &wire.OIDCOptions{ Provider: &wire.Provider{ - IssuerURL: "", + IssuerURL: "https://issuer.example.com", AuthURL: "", TokenURL: "", JWKSURL: "", diff --git a/acme/api/wire_integration_test.go b/acme/api/wire_integration_test.go index 7ac669f4..de4c54c7 100644 --- a/acme/api/wire_integration_test.go +++ b/acme/api/wire_integration_test.go @@ -55,7 +55,7 @@ func TestWireIntegration(t *testing.T) { Wire: &wire.Options{ OIDC: &wire.OIDCOptions{ Provider: &wire.Provider{ - IssuerURL: "", + IssuerURL: "https://issuer.example.com", AuthURL: "", TokenURL: "", JWKSURL: "", diff --git a/acme/challenge.go b/acme/challenge.go index 0b93e579..934aa361 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -370,7 +370,12 @@ func wireOIDC01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSO "error unmarshalling Wire challenge payload")) } - oidcOptions := prov.GetOptions().GetWireOptions().GetOIDCOptions() + wireOptions, err := prov.GetOptions().GetWireOptions() + if err != nil { + return WrapErrorISE(err, "failed getting Wire options") + } + + oidcOptions := wireOptions.GetOIDCOptions() idToken, err := oidcOptions.GetProvider(ctx).Verifier(oidcOptions.GetConfig()).Verify(ctx, oidcPayload.IDToken) if err != nil { return storeError(ctx, db, ch, false, WrapError(ErrorRejectedIdentifierType, err, @@ -474,8 +479,12 @@ func wireDPOP01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSO return WrapErrorISE(err, "error parsing device id") } - dpopOptions := prov.GetOptions().GetWireOptions().GetDPOPOptions() + wireOptions, err := prov.GetOptions().GetWireOptions() + if err != nil { + return WrapErrorISE(err, "failed getting Wire options") + } + dpopOptions := wireOptions.GetDPOPOptions() issuer, err := dpopOptions.EvaluateTarget(clientID.DeviceID) if err != nil { return WrapErrorISE(err, "invalid Go template registered for 'target'") diff --git a/authority/provisioner/options.go b/authority/provisioner/options.go index 1e0457c5..fcb2ca0d 100644 --- a/authority/provisioner/options.go +++ b/authority/provisioner/options.go @@ -2,6 +2,7 @@ package provisioner import ( "encoding/json" + "fmt" "strings" "github.com/pkg/errors" @@ -54,11 +55,14 @@ func (o *Options) GetSSHOptions() *SSHOptions { } // GetWireOptions returns the SSH options. -func (o *Options) GetWireOptions() *wire.Options { +func (o *Options) GetWireOptions() (*wire.Options, error) { if o == nil { - return nil + return nil, errors.New("no Wire options available") } - return o.Wire + if err := o.Wire.Validate(); err != nil { + return nil, fmt.Errorf("failed validating Wire options: %w", err) + } + return o.Wire, nil } // GetWebhooks returns the webhooks options. diff --git a/authority/provisioner/wire/dpop_options.go b/authority/provisioner/wire/dpop_options.go index 5af0c70d..8f39ec7d 100644 --- a/authority/provisioner/wire/dpop_options.go +++ b/authority/provisioner/wire/dpop_options.go @@ -32,14 +32,20 @@ func (o *DPOPOptions) EvaluateTarget(deviceID string) (string, error) { if o == nil { return "", errors.New("misconfigured target template configuration") } - targetTemplate := o.GetTarget() - tmpl, err := template.New("DeviceId").Parse(targetTemplate) + tmpl, err := template.New("DeviceID").Parse(o.GetTarget()) if err != nil { return "", fmt.Errorf("failed parsing dpop template: %w", err) } buf := new(bytes.Buffer) - if err = tmpl.Execute(buf, struct{ DeviceId string }{DeviceId: deviceID}); err != nil { //nolint:revive,stylecheck // TODO(hs): this requires changes in configuration + if err = tmpl.Execute(buf, struct{ DeviceID string }{DeviceID: deviceID}); err != nil { return "", fmt.Errorf("failed executing dpop template: %w", err) } return buf.String(), nil } + +func (o *DPOPOptions) validate() error { + if _, err := template.New("DeviceID").Parse(o.GetTarget()); err != nil { + return fmt.Errorf("failed parsing template: %w", err) + } + return nil +} diff --git a/authority/provisioner/wire/oidc_options.go b/authority/provisioner/wire/oidc_options.go index 64d70398..e44088ba 100644 --- a/authority/provisioner/wire/oidc_options.go +++ b/authority/provisioner/wire/oidc_options.go @@ -59,27 +59,40 @@ func (o *OIDCOptions) GetConfig() *oidc.Config { } } +func (o *OIDCOptions) validate() error { + if o.Provider == nil { + return errors.New("provider not set") + } + if o.Provider.IssuerURL == "" { + return errors.New("issuer URL must not be empty") + } + if _, err := url.Parse(o.Provider.IssuerURL); err != nil { + return fmt.Errorf("failed parsing issuer URL: %w", err) + } + if _, err := template.New("DeviceID").Parse(o.Provider.IssuerURL); err != nil { + return fmt.Errorf("failed parsing template: %w", err) + } + + return nil +} + func (o *OIDCOptions) EvaluateTarget(deviceID string) (string, error) { if o == nil { return "", errors.New("misconfigured target template configuration") } - targetTemplate := o.Provider.IssuerURL - tmpl, err := template.New("DeviceId").Parse(targetTemplate) + tmpl, err := template.New("DeviceID").Parse(o.Provider.IssuerURL) if err != nil { - return "", fmt.Errorf("failed parsing oidc template: %w", err) + return "", fmt.Errorf("failed parsing OIDC template: %w", err) } buf := new(bytes.Buffer) - if err = tmpl.Execute(buf, struct{ DeviceId string }{DeviceId: deviceID}); err != nil { //nolint:revive,stylecheck // TODO(hs): this requires changes in configuration - return "", fmt.Errorf("failed executing oidc template: %w", err) + if err = tmpl.Execute(buf, struct{ DeviceID string }{DeviceID: deviceID}); err != nil { + return "", fmt.Errorf("failed executing OIDC template: %w", err) } return buf.String(), nil } func toOIDCProviderConfig(in *Provider) *oidc.ProviderConfig { - issuerURL, err := url.Parse(in.IssuerURL) - if err != nil { - panic(err) // config error, it's ok to panic here - } + issuerURL, _ := url.Parse(in.IssuerURL) // NOTE: validation is performed in validate() // Removes query params from the URL because we use it as a way to notify client about the actual OAuth ClientId // for this provisioner. // This URL is going to look like: "https://idp:5556/dex?clientid=foo" diff --git a/authority/provisioner/wire/wire_options.go b/authority/provisioner/wire/wire_options.go index 9ab300fb..2042c73f 100644 --- a/authority/provisioner/wire/wire_options.go +++ b/authority/provisioner/wire/wire_options.go @@ -1,9 +1,18 @@ package wire +import ( + "errors" + "fmt" + "sync" +) + // Options holds the Wire ACME extension options type Options struct { OIDC *OIDCOptions `json:"oidc,omitempty"` DPOP *DPOPOptions `json:"dpop,omitempty"` + + validateOnce sync.Once + validationErr error } // GetOIDCOptions returns the OIDC options. @@ -21,3 +30,33 @@ func (o *Options) GetDPOPOptions() *DPOPOptions { } return o.DPOP } + +func (o *Options) Validate() error { + o.validateOnce.Do( + func() { + o.validationErr = validate(o) + }, + ) + + return o.validationErr +} + +func validate(o *Options) error { + if oidc := o.GetOIDCOptions(); oidc != nil { + if err := oidc.validate(); err != nil { + return fmt.Errorf("failed validating OIDC options: %w", err) + } + } else { + return errors.New("no OIDC options available") + } + + if dpop := o.GetDPOPOptions(); dpop != nil { + if err := dpop.validate(); err != nil { + return fmt.Errorf("failed validating DPoP options: %w", err) + } + } else { + return errors.New("no DPoP options available") + } + + return nil +} diff --git a/authority/provisioner/wire/wire_options_test.go b/authority/provisioner/wire/wire_options_test.go new file mode 100644 index 00000000..cc28b142 --- /dev/null +++ b/authority/provisioner/wire/wire_options_test.go @@ -0,0 +1,124 @@ +package wire + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestOptions_Validate(t *testing.T) { + type fields struct { + OIDC *OIDCOptions + DPOP *DPOPOptions + } + tests := []struct { + name string + fields fields + expectedErr error + }{ + { + name: "ok", + fields: fields{ + OIDC: &OIDCOptions{ + Provider: &Provider{ + IssuerURL: "https://example.com", + }, + Config: &Config{}, + }, + DPOP: &DPOPOptions{}, + }, + expectedErr: nil, + }, + { + name: "fail/no-oidc-options", + fields: fields{ + OIDC: nil, + DPOP: &DPOPOptions{}, + }, + expectedErr: errors.New("no OIDC options available"), + }, + { + name: "fail/empty-issuer-url", + fields: fields{ + OIDC: &OIDCOptions{ + Provider: &Provider{ + IssuerURL: "", + }, + Config: &Config{}, + }, + DPOP: &DPOPOptions{}, + }, + expectedErr: errors.New("failed validating OIDC options: issuer URL must not be empty"), + }, + { + name: "fail/invalid-issuer-url", + fields: fields{ + OIDC: &OIDCOptions{ + Provider: &Provider{ + IssuerURL: "\x00", + }, + Config: &Config{}, + }, + DPOP: &DPOPOptions{}, + }, + expectedErr: errors.New(`failed validating OIDC options: failed parsing issuer URL: parse "\x00": net/url: invalid control character in URL`), + }, + { + name: "fail/issuer-url-template", + fields: fields{ + OIDC: &OIDCOptions{ + Provider: &Provider{ + IssuerURL: "https://issuer.example.com/{{}", + }, + Config: &Config{}, + }, + DPOP: &DPOPOptions{}, + }, + expectedErr: errors.New(`failed validating OIDC options: failed parsing template: template: DeviceID:1: unexpected "}" in command`), + }, + { + name: "fail/no-dpop-options", + fields: fields{ + OIDC: &OIDCOptions{ + Provider: &Provider{ + IssuerURL: "https://example.com", + }, + Config: &Config{}, + }, + DPOP: nil, + }, + expectedErr: errors.New("no DPoP options available"), + }, + { + name: "fail/target-template", + fields: fields{ + OIDC: &OIDCOptions{ + Provider: &Provider{ + IssuerURL: "https://example.com", + }, + Config: &Config{}, + }, + DPOP: &DPOPOptions{ + Target: "{{}", + }, + }, + expectedErr: errors.New(`failed validating DPoP options: failed parsing template: template: DeviceID:1: unexpected "}" in command`), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + o := &Options{ + OIDC: tt.fields.OIDC, + DPOP: tt.fields.DPOP, + } + err := o.Validate() + if tt.expectedErr != nil { + assert.EqualError(t, err, tt.expectedErr.Error()) + return + } + + assert.NoError(t, err) + }) + } +}