From c9793561ff5414a41ab7ef093a12cb9ac95848a7 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 24 Oct 2022 14:14:28 +0200 Subject: [PATCH 1/5] Make `meta` object optional in ACME directory response Harware appliances from Kemp seem to validate the contents of the `meta` object, even if none of the properties in the `meta` object is set. According to the RFC, the `meta` object, as well as its properties are optional, so technically this should be fixed by the manufacturer. This commit is to see if we validation of the `meta` object is skipped if it's not available in the response. --- acme/api/handler.go | 15 ++++++++++----- acme/api/handler_test.go | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 6ae57ab8..5a41e4d9 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -205,7 +205,7 @@ type Directory struct { NewOrder string `json:"newOrder"` RevokeCert string `json:"revokeCert"` KeyChange string `json:"keyChange"` - Meta Meta `json:"meta"` + Meta *Meta `json:"meta,omitempty"` } // ToLog enables response logging for the Directory type. @@ -228,16 +228,21 @@ func GetDirectory(w http.ResponseWriter, r *http.Request) { } linker := acme.MustLinkerFromContext(ctx) - render.JSON(w, &Directory{ + directory := &Directory{ NewNonce: linker.GetLink(ctx, acme.NewNonceLinkType), NewAccount: linker.GetLink(ctx, acme.NewAccountLinkType), NewOrder: linker.GetLink(ctx, acme.NewOrderLinkType), RevokeCert: linker.GetLink(ctx, acme.RevokeCertLinkType), KeyChange: linker.GetLink(ctx, acme.KeyChangeLinkType), - Meta: Meta{ + } + // Only add the ACME `meta` object when one (or more) of its + // properties is set. + if acmeProv.RequireEAB { + directory.Meta = &Meta{ ExternalAccountRequired: acmeProv.RequireEAB, - }, - }) + } + } + render.JSON(w, directory) } // NotImplemented returns a 501 and is generally a placeholder for functionality which diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 822409df..15024e5e 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -129,7 +129,7 @@ func TestHandler_GetDirectory(t *testing.T) { NewOrder: fmt.Sprintf("%s/acme/%s/new-order", baseURL.String(), provName), RevokeCert: fmt.Sprintf("%s/acme/%s/revoke-cert", baseURL.String(), provName), KeyChange: fmt.Sprintf("%s/acme/%s/key-change", baseURL.String(), provName), - Meta: Meta{ + Meta: &Meta{ ExternalAccountRequired: true, }, } From b9f238ad4de8f7b4a3d66d0161ee35fc56f0d684 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 24 Oct 2022 22:37:57 +0200 Subject: [PATCH 2/5] Add additional ACME `meta` properties to provisioner configuration --- acme/api/handler.go | 39 +++++++++++++++++++++++++++++------ authority/provisioner/acme.go | 11 ++++++++++ 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 5a41e4d9..d482f869 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -234,15 +234,42 @@ func GetDirectory(w http.ResponseWriter, r *http.Request) { NewOrder: linker.GetLink(ctx, acme.NewOrderLinkType), RevokeCert: linker.GetLink(ctx, acme.RevokeCertLinkType), KeyChange: linker.GetLink(ctx, acme.KeyChangeLinkType), + Meta: createMetaObject(acmeProv), } - // Only add the ACME `meta` object when one (or more) of its - // properties is set. - if acmeProv.RequireEAB { - directory.Meta = &Meta{ - ExternalAccountRequired: acmeProv.RequireEAB, + + render.JSON(w, directory) +} + +// createMetaObject creates a Meta object if the ACME provisioner +// has one or more properties that are written in the ACME directory output. +// It returns nil if none of the properties are set. +func createMetaObject(p *provisioner.ACME) *Meta { + if shouldAddMetaObject(p) { + return &Meta{ + TermsOfService: p.TermsOfService, + Website: p.Website, + CaaIdentities: p.CaaIdentities, + ExternalAccountRequired: p.RequireEAB, } } - render.JSON(w, directory) + return nil +} + +// shouldAddMetaObject returns whether or not the ACME provisioner +// has properties configured that must be added to the ACME directory object. +func shouldAddMetaObject(p *provisioner.ACME) bool { + switch { + case p.TermsOfService != "": + return true + case p.Website != "": + return true + case len(p.CaaIdentities) > 0 && p.CaaIdentities[0] != "": + return true + case p.RequireEAB: + return true + default: + return false + } } // NotImplemented returns a 501 and is generally a placeholder for functionality which diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go index 67a24919..688a3532 100644 --- a/authority/provisioner/acme.go +++ b/authority/provisioner/acme.go @@ -84,6 +84,17 @@ type ACME struct { Type string `json:"type"` Name string `json:"name"` ForceCN bool `json:"forceCN,omitempty"` + // TermsOfService contains a URL pointing to the ACME server's + // terms of service. Defaults to empty. + TermsOfService string `json:"termsOfService,omitempty"` + // Website contains an URL pointing to more information about + // the ACME server. Defaults to empty. + Website string `json:"website,omitempty"` + // CaaIdentities is an array of hostnames that the ACME server + // identifies itself with. These hostnames can be used by ACME + // clients to determine the correct issuer domain name to use + // when configuring CAA records. Defaults to empty array. + CaaIdentities []string `json:"caaIdentities,omitempty"` // RequireEAB makes the provisioner require ACME EAB to be provided // by clients when creating a new Account. If set to true, the provided // EAB will be verified. If set to false and an EAB is provided, it is From 3eae04928fbda28215959a79c925aed68faf0d41 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 7 Nov 2022 15:35:42 +0100 Subject: [PATCH 3/5] Add tests for ACME Meta object --- acme/api/handler.go | 9 ++- acme/api/handler_test.go | 121 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 7 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index d482f869..776f012b 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -228,16 +228,15 @@ func GetDirectory(w http.ResponseWriter, r *http.Request) { } linker := acme.MustLinkerFromContext(ctx) - directory := &Directory{ + + render.JSON(w, &Directory{ NewNonce: linker.GetLink(ctx, acme.NewNonceLinkType), NewAccount: linker.GetLink(ctx, acme.NewAccountLinkType), NewOrder: linker.GetLink(ctx, acme.NewOrderLinkType), RevokeCert: linker.GetLink(ctx, acme.RevokeCertLinkType), KeyChange: linker.GetLink(ctx, acme.KeyChangeLinkType), Meta: createMetaObject(acmeProv), - } - - render.JSON(w, directory) + }) } // createMetaObject creates a Meta object if the ACME provisioner @@ -263,7 +262,7 @@ func shouldAddMetaObject(p *provisioner.ACME) bool { return true case p.Website != "": return true - case len(p.CaaIdentities) > 0 && p.CaaIdentities[0] != "": + case len(p.CaaIdentities) > 0: return true case p.RequireEAB: return true diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 15024e5e..1edeb501 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -18,10 +18,13 @@ import ( "github.com/go-chi/chi" "github.com/google/go-cmp/cmp" "github.com/pkg/errors" - "github.com/smallstep/assert" - "github.com/smallstep/certificates/acme" + "go.step.sm/crypto/jose" "go.step.sm/crypto/pemutil" + + "github.com/smallstep/assert" + "github.com/smallstep/certificates/acme" + "github.com/smallstep/certificates/authority/provisioner" ) type mockClient struct { @@ -139,6 +142,34 @@ func TestHandler_GetDirectory(t *testing.T) { statusCode: 200, } }, + "ok/full-meta": func(t *testing.T) test { + prov := newACMEProv(t) + prov.TermsOfService = "https://terms.ca.local/" + prov.Website = "https://ca.local/" + prov.CaaIdentities = []string{"ca.local"} + prov.RequireEAB = true + provName := url.PathEscape(prov.GetName()) + baseURL := &url.URL{Scheme: "https", Host: "test.ca.smallstep.com"} + ctx := acme.NewProvisionerContext(context.Background(), prov) + expDir := Directory{ + NewNonce: fmt.Sprintf("%s/acme/%s/new-nonce", baseURL.String(), provName), + NewAccount: fmt.Sprintf("%s/acme/%s/new-account", baseURL.String(), provName), + NewOrder: fmt.Sprintf("%s/acme/%s/new-order", baseURL.String(), provName), + RevokeCert: fmt.Sprintf("%s/acme/%s/revoke-cert", baseURL.String(), provName), + KeyChange: fmt.Sprintf("%s/acme/%s/key-change", baseURL.String(), provName), + Meta: &Meta{ + TermsOfService: "https://terms.ca.local/", + Website: "https://ca.local/", + CaaIdentities: []string{"ca.local"}, + ExternalAccountRequired: true, + }, + } + return test{ + ctx: ctx, + dir: expDir, + statusCode: 200, + } + }, } for name, run := range tests { tc := run(t) @@ -751,3 +782,89 @@ func TestHandler_GetChallenge(t *testing.T) { }) } } + +func Test_createMetaObject(t *testing.T) { + tests := []struct { + name string + p *provisioner.ACME + want *Meta + }{ + { + name: "no-meta", + p: &provisioner.ACME{ + Type: "ACME", + Name: "acme", + }, + want: nil, + }, + { + name: "terms-of-service", + p: &provisioner.ACME{ + Type: "ACME", + Name: "acme", + TermsOfService: "https://terms.ca.local", + }, + want: &Meta{ + TermsOfService: "https://terms.ca.local", + }, + }, + { + name: "website", + p: &provisioner.ACME{ + Type: "ACME", + Name: "acme", + Website: "https://ca.local", + }, + want: &Meta{ + Website: "https://ca.local", + }, + }, + { + name: "caa", + p: &provisioner.ACME{ + Type: "ACME", + Name: "acme", + CaaIdentities: []string{"ca.local", "ca.remote"}, + }, + want: &Meta{ + CaaIdentities: []string{"ca.local", "ca.remote"}, + }, + }, + { + name: "require-eab", + p: &provisioner.ACME{ + Type: "ACME", + Name: "acme", + RequireEAB: true, + }, + want: &Meta{ + ExternalAccountRequired: true, + }, + }, + { + name: "full-meta", + p: &provisioner.ACME{ + Type: "ACME", + Name: "acme", + TermsOfService: "https://terms.ca.local", + Website: "https://ca.local", + CaaIdentities: []string{"ca.local", "ca.remote"}, + RequireEAB: true, + }, + want: &Meta{ + TermsOfService: "https://terms.ca.local", + Website: "https://ca.local", + CaaIdentities: []string{"ca.local", "ca.remote"}, + ExternalAccountRequired: true, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := createMetaObject(tt.p) + if !cmp.Equal(tt.want, got) { + t.Errorf("createMetaObject() diff =\n%s", cmp.Diff(tt.want, got)) + } + }) + } +} From a7b2f5f27d5551c51b6744138690e951262506c6 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 7 Nov 2022 22:14:10 +0100 Subject: [PATCH 4/5] Upgrade `linkedca` to `v0.19.0-rc.4` --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 45e08723..99905df7 100644 --- a/go.mod +++ b/go.mod @@ -44,7 +44,7 @@ require ( go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352 go.step.sm/cli-utils v0.7.5 go.step.sm/crypto v0.23.0 - go.step.sm/linkedca v0.19.0-rc.3 + go.step.sm/linkedca v0.19.0-rc.4 golang.org/x/crypto v0.0.0-20221005025214-4161e89ecf1b golang.org/x/net v0.0.0-20221014081412-f15817d10f9b golang.org/x/sys v0.0.0-20221006211917-84dc82d7e875 // indirect diff --git a/go.sum b/go.sum index 5584e149..2a3e80b9 100644 --- a/go.sum +++ b/go.sum @@ -688,6 +688,8 @@ go.step.sm/crypto v0.23.0 h1:pkkAlQxeDs+7qZ0mWSnN25qbtDm/AH6u0hYlwcmRWng= go.step.sm/crypto v0.23.0/go.mod h1:sK4iH/xyQDbffE1jCgj5hraVrbdKY9CTs0Lnjskxnk4= go.step.sm/linkedca v0.19.0-rc.3 h1:3Uu8j187wm7mby+/pz/aQ0wHKRm7w/2AsVPpvcAn4v8= go.step.sm/linkedca v0.19.0-rc.3/go.mod h1:MCZmPIdzElEofZbiw4eyUHayXgFTwa94cNAV34aJ5ew= +go.step.sm/linkedca v0.19.0-rc.4 h1:kaBW+xHkRRgMNDa4gWiIj7gBq5yjbJKGlTWYYo5z2KQ= +go.step.sm/linkedca v0.19.0-rc.4/go.mod h1:b7vWPrHfYLEOTSUZitFEcztVCpTc+ileIN85CwEAluM= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ= From 920c4f02c54a3b3905dd175d2f8ae1fb6ae12e88 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 7 Nov 2022 22:34:21 +0100 Subject: [PATCH 5/5] Add additional properties to provisioner converters --- authority/provisioners.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/authority/provisioners.go b/authority/provisioners.go index bfa4eae5..4220b07d 100644 --- a/authority/provisioners.go +++ b/authority/provisioners.go @@ -861,6 +861,9 @@ func ProvisionerToCertificates(p *linkedca.Provisioner) (provisioner.Interface, Type: p.Type.String(), Name: p.Name, ForceCN: cfg.ForceCn, + TermsOfService: cfg.TermsOfService, + Website: cfg.Website, + CaaIdentities: cfg.CaaIdentities, RequireEAB: cfg.RequireEab, Challenges: challengesToCertificates(cfg.Challenges), AttestationFormats: attestationFormatsToCertificates(cfg.AttestationFormats), @@ -1119,6 +1122,10 @@ func ProvisionerToLinkedca(p provisioner.Interface) (*linkedca.Provisioner, erro Data: &linkedca.ProvisionerDetails_ACME{ ACME: &linkedca.ACMEProvisioner{ ForceCn: p.ForceCN, + TermsOfService: p.TermsOfService, + Website: p.Website, + CaaIdentities: p.CaaIdentities, + RequireEab: p.RequireEAB, Challenges: challengesToLinkedca(p.Challenges), AttestationFormats: attestationFormatsToLinkedca(p.AttestationFormats), AttestationRoots: provisionerPEMToLinkedca(p.AttestationRoots),