Skip to content

Commit

Permalink
Require HS256, HS384, or HS512 for EAB (#459)
Browse files Browse the repository at this point in the history
During the go-jose/v4 upgrade, I accidentally required the same
signature sets for EAB as for the account keys, which is incorrect. This
allows the correct MAC-based algorithms. It drops the custom algorithm
checks, which are now unreachable as go-jose will enforce the
algorithms.

This also adds a new EAB key to Pebble's test config which explicitly
has base64url characters, from #428 

Fixes #455

---------

Co-authored-by: Folke Gleumes <folke@gleumes.org>
  • Loading branch information
mcpherrinm and j0ru authored May 1, 2024
1 parent 8250e65 commit e87ace7
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 10 deletions.
27 changes: 27 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,33 @@ jobs:
run: |
git clone https://github.com/eggsampler/acme.git /tmp/eggsampler-acme
cd /tmp/eggsampler-acme && make test
lego-eab-linux:
name: Test lego with EAB
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version-file: go.mod
- name: Install lego cli
run: go install github.com/go-acme/lego/v4/cmd/lego@latest
- name: go install commands
run: go install -v ./cmd/...
- name: launch pebble
run: |
GORACE="halt_on_error=1" PEBBLE_VA_ALWAYS_VALID=1 \
pebble -config test/config/pebble-config-external-account-bindings.json &
- run: |
LEGO_CA_CERTIFICATES=./test/certs/pebble.minica.pem \
lego --accept-tos \
--server=https://localhost:14000/dir \
--email="pebble-test@example.letsencrypt.org" \
--domains=example.letsencrypt.org \
--eab \
--kid kid-3 \
--hmac=HjudV5qnbreN-n9WyFSH-t4HXuEx_XFen45zuxY-G1h6fr74V3cUM_dVlwQZBWmc \
--http --http.port=:5002 \
run
go-linux:
name: Run Go tests on Linux
runs-on: ubuntu-latest
Expand Down
3 changes: 2 additions & 1 deletion test/config/pebble-config-external-account-bindings.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"externalAccountBindingRequired": true,
"externalAccountMACKeys": {
"kid-1": "zWNDZM6eQGHWpSRTPal5eIUYFTu7EajVIoguysqZ9wG44nMEtx3MUAsUDkMTQ12W",
"kid-2": "b10lLJs8l1GPIzsLP0s6pMt8O0XVGnfTaCeROxQM0BIt2XrJMDHJZBM5NuQmQJQH"
"kid-2": "b10lLJs8l1GPIzsLP0s6pMt8O0XVGnfTaCeROxQM0BIt2XrJMDHJZBM5NuQmQJQH",
"kid-3": "HjudV5qnbreN-n9WyFSH-t4HXuEx_XFen45zuxY-G1h6fr74V3cUM_dVlwQZBWmc"
},
"certificateValidityPeriod": 157766400
}
Expand Down
15 changes: 6 additions & 9 deletions wfe/wfe.go
Original file line number Diff line number Diff line change
Expand Up @@ -2786,14 +2786,18 @@ func (wfe *WebFrontEndImpl) verifyEAB(
fmt.Sprintf("failed to encode external account binding JSON structure: %s", err))
}

eab, err := jose.ParseSigned(string(eabBytes), goodJWSSignatureAlgorithms)
// The "alg" field MUST indicate a MAC-based algorithm
eabSignatureAlgorithms := []jose.SignatureAlgorithm{
jose.HS256, jose.HS384, jose.HS512,
}

eab, err := jose.ParseSigned(string(eabBytes), eabSignatureAlgorithms)
if err != nil {
return nil, acme.MalformedProblem(
fmt.Sprintf("failed to decode external account binding: %s", err))
}

// 2. Verify that the JWS protected field meets the following criteria
// - The "alg" field MUST indicate a MAC-based algorithm
// - The "kid" field MUST contain the key identifier provided by the CA
// - The "nonce" field MUST NOT be present
// - The "url" field MUST be set to the same value as the outer JWS
Expand Down Expand Up @@ -2839,13 +2843,6 @@ func (wfe *WebFrontEndImpl) verifyEABPayloadHeader(innerJWS *jose.JSONWebSignatu
}

header := innerJWS.Signatures[0].Protected
switch header.Algorithm {
case "HS256", "HS384", "HS512":
break
default:
return "", acme.BadPublicKeyProblem(
fmt.Sprintf("the 'alg' field is set to %q, which is not valid for external account binding, valid values are: HS256, HS384 or HS512", header.Algorithm))
}
if len(header.Nonce) > 0 {
return "", acme.MalformedProblem(
"the 'nonce' field must be absent in external account binding")
Expand Down

0 comments on commit e87ace7

Please sign in to comment.