Skip to content

Commit ef951f2

Browse files
hslatmanhslatman
authored
Merge pull request smallstep#1204 from smallstep/herman/improve-scep-marshaling
Improve SCEP provisioner marshaling
2 parents eba93da + 8c53dc9 commit ef951f2

File tree

11 files changed

+375
-40
lines changed

11 files changed

+375
-40
lines changed

‎api/api.go

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -224,8 +224,39 @@ type RootResponse struct {
224224
// ProvisionersResponse is the response object that returns the list of
225225
// provisioners.
226226
type ProvisionersResponse struct {
227-
Provisioners provisioner.List `json:"provisioners"`
228-
NextCursor string `json:"nextCursor"`
227+
Provisioners provisioner.List
228+
NextCursor string
229+
}
230+
231+
// MarshalJSON implements json.Marshaler. It marshals the ProvisionersResponse
232+
// into a byte slice.
233+
//
234+
// Special treatment is given to the SCEP provisioner, as it contains a
235+
// challenge secret that MUST NOT be leaked in (public) HTTP responses. The
236+
// challenge value is thus redacted in HTTP responses.
237+
func (p ProvisionersResponse) MarshalJSON() ([]byte, error) {
238+
for _, item := range p.Provisioners {
239+
scepProv, ok := item.(*provisioner.SCEP)
240+
if !ok {
241+
continue
242+
}
243+
244+
old := scepProv.ChallengePassword
245+
scepProv.ChallengePassword = "*** REDACTED ***"
246+
defer func(p string) { //nolint:gocritic // defer in loop required to restore initial state of provisioners
247+
scepProv.ChallengePassword = p
248+
}(old)
249+
}
250+
251+
var list = struct {
252+
Provisioners []provisioner.Interface `json:"provisioners"`
253+
NextCursor string `json:"nextCursor"`
254+
}{
255+
Provisioners: []provisioner.Interface(p.Provisioners),
256+
NextCursor: p.NextCursor,
257+
}
258+
259+
return json.Marshal(list)
229260
}
230261

231262
// ProvisionerKeyResponse is the response object that returns the encrypted key

‎api/api_test.go

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import (
44
"bytes"
55
"context"
66
"crypto"
7-
"crypto/dsa" //nolint
7+
"crypto/dsa" //nolint:staticcheck // support legacy algorithms
88
"crypto/ecdsa"
99
"crypto/ed25519"
1010
"crypto/elliptic"
@@ -28,7 +28,9 @@ import (
2828

2929
"github.com/go-chi/chi"
3030
"github.com/pkg/errors"
31+
sassert "github.com/stretchr/testify/assert"
3132
"golang.org/x/crypto/ssh"
33+
squarejose "gopkg.in/square/go-jose.v2"
3234

3335
"go.step.sm/crypto/jose"
3436
"go.step.sm/crypto/x509util"
@@ -1564,3 +1566,94 @@ func mustCertificate(t *testing.T, pub, priv interface{}) *x509.Certificate {
15641566
}
15651567
return cert
15661568
}
1569+
1570+
func TestProvisionersResponse_MarshalJSON(t *testing.T) {
1571+
1572+
k := map[string]any{
1573+
"use": "sig",
1574+
"kty": "EC",
1575+
"kid": "4UELJx8e0aS9m0CH3fZ0EB7D5aUPICb759zALHFejvc",
1576+
"crv": "P-256",
1577+
"alg": "ES256",
1578+
"x": "7ZdAAMZCFU4XwgblI5RfZouBi8lYmF6DlZusNNnsbm8",
1579+
"y": "sQr2JdzwD2fgyrymBEXWsxDxFNjjqN64qLLSbLdLZ9Y",
1580+
}
1581+
key := squarejose.JSONWebKey{}
1582+
b, err := json.Marshal(k)
1583+
assert.FatalError(t, err)
1584+
err = json.Unmarshal(b, &key)
1585+
assert.FatalError(t, err)
1586+
1587+
r := ProvisionersResponse{
1588+
Provisioners: provisioner.List{
1589+
&provisioner.SCEP{
1590+
Name: "scep",
1591+
Type: "scep",
1592+
ChallengePassword: "not-so-secret",
1593+
MinimumPublicKeyLength: 2048,
1594+
EncryptionAlgorithmIdentifier: 2,
1595+
},
1596+
&provisioner.JWK{
1597+
EncryptedKey: "eyJhbGciOiJQQkVTMi1IUzI1NitBMTI4S1ciLCJlbmMiOiJBMTI4R0NNIiwicDJjIjoxMDAwMDAsInAycyI6IlhOdmYxQjgxSUlLMFA2NUkwcmtGTGcifQ.XaN9zcPQeWt49zchUDm34FECUTHfQTn_.tmNHPQDqR3ebsWfd.9WZr3YVdeOyJh36vvx0VlRtluhvYp4K7jJ1KGDr1qypwZ3ziBVSNbYYQ71du7fTtrnfG1wgGTVR39tWSzBU-zwQ5hdV3rpMAaEbod5zeW6SHd95H3Bvcb43YiiqJFNL5sGZzFb7FqzVmpsZ1efiv6sZaGDHtnCAL6r12UG5EZuqGfM0jGCZitUz2m9TUKXJL5DJ7MOYbFfkCEsUBPDm_TInliSVn2kMJhFa0VOe5wZk5YOuYM3lNYW64HGtbf-llN2Xk-4O9TfeSPizBx9ZqGpeu8pz13efUDT2WL9tWo6-0UE-CrG0bScm8lFTncTkHcu49_a5NaUBkYlBjEiw.thPcx3t1AUcWuEygXIY3Fg",
1598+
Key: &key,
1599+
Name: "step-cli",
1600+
Type: "JWK",
1601+
},
1602+
},
1603+
NextCursor: "next",
1604+
}
1605+
1606+
expected := map[string]any{
1607+
"provisioners": []map[string]any{
1608+
{
1609+
"type": "scep",
1610+
"name": "scep",
1611+
"challenge": "*** REDACTED ***",
1612+
"minimumPublicKeyLength": 2048,
1613+
"encryptionAlgorithmIdentifier": 2,
1614+
},
1615+
{
1616+
"type": "JWK",
1617+
"name": "step-cli",
1618+
"key": map[string]any{
1619+
"use": "sig",
1620+
"kty": "EC",
1621+
"kid": "4UELJx8e0aS9m0CH3fZ0EB7D5aUPICb759zALHFejvc",
1622+
"crv": "P-256",
1623+
"alg": "ES256",
1624+
"x": "7ZdAAMZCFU4XwgblI5RfZouBi8lYmF6DlZusNNnsbm8",
1625+
"y": "sQr2JdzwD2fgyrymBEXWsxDxFNjjqN64qLLSbLdLZ9Y",
1626+
},
1627+
"encryptedKey": "eyJhbGciOiJQQkVTMi1IUzI1NitBMTI4S1ciLCJlbmMiOiJBMTI4R0NNIiwicDJjIjoxMDAwMDAsInAycyI6IlhOdmYxQjgxSUlLMFA2NUkwcmtGTGcifQ.XaN9zcPQeWt49zchUDm34FECUTHfQTn_.tmNHPQDqR3ebsWfd.9WZr3YVdeOyJh36vvx0VlRtluhvYp4K7jJ1KGDr1qypwZ3ziBVSNbYYQ71du7fTtrnfG1wgGTVR39tWSzBU-zwQ5hdV3rpMAaEbod5zeW6SHd95H3Bvcb43YiiqJFNL5sGZzFb7FqzVmpsZ1efiv6sZaGDHtnCAL6r12UG5EZuqGfM0jGCZitUz2m9TUKXJL5DJ7MOYbFfkCEsUBPDm_TInliSVn2kMJhFa0VOe5wZk5YOuYM3lNYW64HGtbf-llN2Xk-4O9TfeSPizBx9ZqGpeu8pz13efUDT2WL9tWo6-0UE-CrG0bScm8lFTncTkHcu49_a5NaUBkYlBjEiw.thPcx3t1AUcWuEygXIY3Fg",
1628+
},
1629+
},
1630+
"nextCursor": "next",
1631+
}
1632+
1633+
expBytes, err := json.Marshal(expected)
1634+
sassert.NoError(t, err)
1635+
1636+
br, err := r.MarshalJSON()
1637+
sassert.NoError(t, err)
1638+
sassert.JSONEq(t, string(expBytes), string(br))
1639+
1640+
keyCopy := key
1641+
expList := provisioner.List{
1642+
&provisioner.SCEP{
1643+
Name: "scep",
1644+
Type: "scep",
1645+
ChallengePassword: "not-so-secret",
1646+
MinimumPublicKeyLength: 2048,
1647+
EncryptionAlgorithmIdentifier: 2,
1648+
},
1649+
&provisioner.JWK{
1650+
EncryptedKey: "eyJhbGciOiJQQkVTMi1IUzI1NitBMTI4S1ciLCJlbmMiOiJBMTI4R0NNIiwicDJjIjoxMDAwMDAsInAycyI6IlhOdmYxQjgxSUlLMFA2NUkwcmtGTGcifQ.XaN9zcPQeWt49zchUDm34FECUTHfQTn_.tmNHPQDqR3ebsWfd.9WZr3YVdeOyJh36vvx0VlRtluhvYp4K7jJ1KGDr1qypwZ3ziBVSNbYYQ71du7fTtrnfG1wgGTVR39tWSzBU-zwQ5hdV3rpMAaEbod5zeW6SHd95H3Bvcb43YiiqJFNL5sGZzFb7FqzVmpsZ1efiv6sZaGDHtnCAL6r12UG5EZuqGfM0jGCZitUz2m9TUKXJL5DJ7MOYbFfkCEsUBPDm_TInliSVn2kMJhFa0VOe5wZk5YOuYM3lNYW64HGtbf-llN2Xk-4O9TfeSPizBx9ZqGpeu8pz13efUDT2WL9tWo6-0UE-CrG0bScm8lFTncTkHcu49_a5NaUBkYlBjEiw.thPcx3t1AUcWuEygXIY3Fg",
1651+
Key: &keyCopy,
1652+
Name: "step-cli",
1653+
Type: "JWK",
1654+
},
1655+
}
1656+
1657+
// MarshalJSON must not affect the struct properties itself
1658+
sassert.Equal(t, expList, r.Provisioners)
1659+
}

‎authority/provisioner/provisioner.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@ import (
1010
"strings"
1111

1212
"github.com/pkg/errors"
13-
"github.com/smallstep/certificates/errs"
1413
"golang.org/x/crypto/ssh"
14+
15+
"github.com/smallstep/certificates/errs"
1516
)
1617

1718
// Interface is the interface that all provisioner types must implement.

‎authority/provisioner/scep.go

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ type SCEP struct {
3939
Options *Options `json:"options,omitempty"`
4040
Claims *Claims `json:"claims,omitempty"`
4141
ctl *Controller
42-
secretChallengePassword string
4342
encryptionAlgorithm int
4443
challengeValidationController *challengeValidationController
4544
}
@@ -159,10 +158,6 @@ func (s *SCEP) Init(config Config) (err error) {
159158
return errors.New("provisioner name cannot be empty")
160159
}
161160

162-
// Mask the actual challenge value, so it won't be marshaled
163-
s.secretChallengePassword = s.ChallengePassword
164-
s.ChallengePassword = "*** redacted ***"
165-
166161
// Default to 2048 bits minimum public key length (for CSRs) if not set
167162
if s.MinimumPublicKeyLength == 0 {
168163
s.MinimumPublicKeyLength = 2048
@@ -206,11 +201,6 @@ func (s *SCEP) AuthorizeSign(ctx context.Context, token string) ([]SignOption, e
206201
}, nil
207202
}
208203

209-
// GetChallengePassword returns the challenge password
210-
func (s *SCEP) GetChallengePassword() string {
211-
return s.secretChallengePassword
212-
}
213-
214204
// GetCapabilities returns the CA capabilities
215205
func (s *SCEP) GetCapabilities() []string {
216206
return s.Capabilities
@@ -241,7 +231,7 @@ func (s *SCEP) ValidateChallenge(ctx context.Context, challenge, transactionID s
241231
case validationMethodWebhook:
242232
return s.challengeValidationController.Validate(ctx, challenge, transactionID)
243233
default:
244-
if subtle.ConstantTimeCompare([]byte(s.secretChallengePassword), []byte(challenge)) == 0 {
234+
if subtle.ConstantTimeCompare([]byte(s.ChallengePassword), []byte(challenge)) == 0 {
245235
return errors.New("invalid challenge password provided")
246236
}
247237
return nil
@@ -264,7 +254,7 @@ func (s *SCEP) selectValidationMethod() validationMethod {
264254
if len(s.challengeValidationController.webhooks) > 0 {
265255
return validationMethodWebhook
266256
}
267-
if s.secretChallengePassword != "" {
257+
if s.ChallengePassword != "" {
268258
return validationMethodStatic
269259
}
270260
return validationMethodNone

‎authority/provisioners.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1223,7 +1223,7 @@ func ProvisionerToLinkedca(p provisioner.Interface) (*linkedca.Provisioner, erro
12231223
Data: &linkedca.ProvisionerDetails_SCEP{
12241224
SCEP: &linkedca.SCEPProvisioner{
12251225
ForceCn: p.ForceCN,
1226-
Challenge: p.GetChallengePassword(),
1226+
Challenge: p.ChallengePassword,
12271227
Capabilities: p.Capabilities,
12281228
MinimumPublicKeyLength: int32(p.MinimumPublicKeyLength),
12291229
IncludeRoot: p.IncludeRoot,

‎authority/provisioners_test.go

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,17 @@ import (
99
"testing"
1010
"time"
1111

12+
"go.step.sm/crypto/jose"
13+
"go.step.sm/crypto/keyutil"
14+
"go.step.sm/linkedca"
15+
16+
"github.com/stretchr/testify/require"
17+
1218
"github.com/smallstep/assert"
1319
"github.com/smallstep/certificates/api/render"
1420
"github.com/smallstep/certificates/authority/admin"
1521
"github.com/smallstep/certificates/authority/provisioner"
1622
"github.com/smallstep/certificates/db"
17-
"go.step.sm/crypto/jose"
18-
"go.step.sm/crypto/keyutil"
19-
"go.step.sm/linkedca"
2023
)
2124

2225
func TestGetEncryptedKey(t *testing.T) {
@@ -29,19 +32,19 @@ func TestGetEncryptedKey(t *testing.T) {
2932
tests := map[string]func(t *testing.T) *ek{
3033
"ok": func(t *testing.T) *ek {
3134
c, err := LoadConfiguration("../ca/testdata/ca.json")
32-
assert.FatalError(t, err)
35+
require.NoError(t, err)
3336
a, err := New(c)
34-
assert.FatalError(t, err)
37+
require.NoError(t, err)
3538
return &ek{
3639
a: a,
3740
kid: c.AuthorityConfig.Provisioners[1].(*provisioner.JWK).Key.KeyID,
3841
}
3942
},
4043
"fail-not-found": func(t *testing.T) *ek {
4144
c, err := LoadConfiguration("../ca/testdata/ca.json")
42-
assert.FatalError(t, err)
45+
require.NoError(t, err)
4346
a, err := New(c)
44-
assert.FatalError(t, err)
47+
require.NoError(t, err)
4548
return &ek{
4649
a: a,
4750
kid: "foo",
@@ -95,9 +98,16 @@ func TestGetProvisioners(t *testing.T) {
9598
tests := map[string]func(t *testing.T) *gp{
9699
"ok": func(t *testing.T) *gp {
97100
c, err := LoadConfiguration("../ca/testdata/ca.json")
98-
assert.FatalError(t, err)
101+
require.NoError(t, err)
99102
a, err := New(c)
100-
assert.FatalError(t, err)
103+
require.NoError(t, err)
104+
return &gp{a: a}
105+
},
106+
"ok/rsa": func(t *testing.T) *gp {
107+
c, err := LoadConfiguration("../ca/testdata/rsaca.json")
108+
require.NoError(t, err)
109+
a, err := New(c)
110+
require.NoError(t, err)
101111
return &gp{a: a}
102112
},
103113
}
@@ -111,13 +121,13 @@ func TestGetProvisioners(t *testing.T) {
111121
if assert.NotNil(t, tc.err) {
112122
var sc render.StatusCodedError
113123
if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") {
114-
assert.Equals(t, sc.StatusCode(), tc.code)
124+
assert.Equals(t, tc.code, sc.StatusCode())
115125
}
116-
assert.HasPrefix(t, err.Error(), tc.err.Error())
126+
assert.HasPrefix(t, tc.err.Error(), err.Error())
117127
}
118128
} else {
119129
if assert.Nil(t, tc.err) {
120-
assert.Equals(t, ps, tc.a.config.AuthorityConfig.Provisioners)
130+
assert.Equals(t, tc.a.config.AuthorityConfig.Provisioners, ps)
121131
assert.Equals(t, "", next)
122132
}
123133
}
@@ -127,20 +137,20 @@ func TestGetProvisioners(t *testing.T) {
127137

128138
func TestAuthority_LoadProvisionerByCertificate(t *testing.T) {
129139
_, priv, err := keyutil.GenerateDefaultKeyPair()
130-
assert.FatalError(t, err)
140+
require.NoError(t, err)
131141
csr := getCSR(t, priv)
132142

133143
sign := func(a *Authority, extraOpts ...provisioner.SignOption) *x509.Certificate {
134144
key, err := jose.ReadKey("testdata/secrets/step_cli_key_priv.jwk", jose.WithPassword([]byte("pass")))
135-
assert.FatalError(t, err)
145+
require.NoError(t, err)
136146
token, err := generateToken("smallstep test", "step-cli", testAudiences.Sign[0], []string{"test.smallstep.com"}, time.Now(), key)
137-
assert.FatalError(t, err)
147+
require.NoError(t, err)
138148
ctx := provisioner.NewContextWithMethod(context.Background(), provisioner.SignMethod)
139149
opts, err := a.Authorize(ctx, token)
140-
assert.FatalError(t, err)
150+
require.NoError(t, err)
141151
opts = append(opts, extraOpts...)
142152
certs, err := a.Sign(csr, provisioner.SignOptions{}, opts...)
143-
assert.FatalError(t, err)
153+
require.NoError(t, err)
144154
return certs[0]
145155
}
146156
getProvisioner := func(a *Authority, name string) provisioner.Interface {
@@ -169,9 +179,7 @@ func TestAuthority_LoadProvisionerByCertificate(t *testing.T) {
169179
},
170180
MGetCertificateData: func(serialNumber string) (*db.CertificateData, error) {
171181
p, err := a1.LoadProvisionerByName("dev")
172-
if err != nil {
173-
t.Fatal(err)
174-
}
182+
require.NoError(t, err)
175183
return &db.CertificateData{
176184
Provisioner: &db.ProvisionerData{
177185
ID: p.GetID(),
@@ -186,9 +194,7 @@ func TestAuthority_LoadProvisionerByCertificate(t *testing.T) {
186194
a2.adminDB = &mockAdminDB{
187195
MGetCertificateData: (func(s string) (*db.CertificateData, error) {
188196
p, err := a2.LoadProvisionerByName("dev")
189-
if err != nil {
190-
t.Fatal(err)
191-
}
197+
require.NoError(t, err)
192198
return &db.CertificateData{
193199
Provisioner: &db.ProvisionerData{
194200
ID: p.GetID(),

0 commit comments

Comments
 (0)