Upgrade from go/go-jose.v2 to go-jose/go-jose.v3 #6581

pgporada · 2023-01-11T20:24:38Z

Why do we need this?

The square/go-jose repository has been deprecated and replaced by go-jose/go-jose. Additionally, work on square/go-jose v2 has stopped. Further work will be done on go-jose/go-jose v3.

According to the ACME RFC, Boulder uses JSON Web Signature (JWS) RFC 7515 for request URL integrity, request authentication, and replay protection via nonces.

What functionality actually changed in this upgrade?
Great question, here's the release notes. https://github.com/go-jose/go-jose/releases/tag/v3.0.0

I noticed a comment that seems like it was left over from an ACMEv1 divergence. This divergence is not listed for ACMEv2.

boulder/web/context.go

Lines 142 to 145 in b21f9b7

    
           // The main reason we want to strip these ports out is so that when this header 
        
           // is sent to the /directory endpoint we don't reply with directory URLs that 
        
           // also contain these ports, which would then in turn end up being sent in the JWS 
        
           // signature 'url' header, which we don't support.

// The main reason we want to strip these ports out is so that when this header
// is sent to the /directory endpoint we don't reply with directory URLs that
// also contain these ports, which would then in turn end up being sent in the JWS
// signature 'url' header, which we don't support.

Boulder core/objects.go already handles a fix for stripping padding off base64 strings that go-jose/go-jose/v3 implemented.

ACME RFC 8555 section 6.2 states that the JWS payload must not be detached, but go-jose/go-jose/v3 now supports that for JWKs which is cool I guess.

We're not using the jose-util tools as far as I can tell.

None of the other commits in go-jose/go-jose/v3 appear to be relevant to boulder.

Here's the crux of the upgrade.

$ grep -rli 'gopkg.in/square/go-jose.v2' 2>&1 | grep -v grep | grep -v '.gocache' | grep -v 'bin/' | grep -v vendor | grep -v '.git' | xargs -i@ sed -i 's|gopkg.in/square/go-jose.v2|github.com/go-jose/go-jose/v3|g' @
$ go mod vendor
$ go mod tidy
$ docker compose run --use-aliases boulder ./test.sh

Running Lints
core/objects.go:14: File is not `gofmt`-ed with `-s` (gofmt)
	"golang.org/x/crypto/ocsp"
core/core_test.go:8: File is not `gofmt`-ed with `-s` (gofmt)
	"github.com/letsencrypt/boulder/test"
test/load-generator/boulder-calls.go:22: File is not `gofmt`-ed with `-s` (gofmt)

FAILURE while running lints

There were a lot of lint failures that gofmt fixed for me and our test suite ended up passing successfully.

I noticed that vendored dependencies don't download the ${module}_test.go files, so to test the new go-jose version per CONTRIBUTING.md I ran:

$ git clone https://github.com/go-jose/go-jose
$ cd go-jose
$ git branch -l
$ go test
go: downloading golang.org/x/crypto v0.0.0-20190911031432-227b76d455e7
go: downloading github.com/google/go-cmp v0.5.0
go: downloading github.com/stretchr/testify v1.6.1
go: downloading github.com/davecgh/go-spew v1.1.0
go: downloading gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c
PASS
ok  	github.com/go-jose/go-jose/v3	20.982s

Fixes #6573

aarongable

LGTM!

Why do we need this?

I'd also add to this section something along the lines of "the square/go-jose repository is being deprecated and replaced by go-jose/go-jose", to explain why we're making this change as opposed to just why we need this dependency at all.

I noticed a comment that seems like it was left over from an ACMEv1 divergence.

Ooh, we should just delete that doc entirely! ACMEv1 is dead, long live ACME, etc etc. Wanna file a PR for that?

Boulder core/objects.go already handles a fix for stripping padding off base64 strings that go-jose/go-jose/v3 implemented.

Interesting! Two small notes here:

It's just interesting to see how these accomplish the same goal but using opposite tools: the go-jose version strips trailing = and then uses base64.RawURLEncoding, while the Boulder version adds the right number of = and then uses base64.URLEncoding. Frankly the go-jose version is "more correct" because clients are supposed to be stripping those padding characters themselves, so I'm changing the Boulder version here.
These actually serve slightly different purposes: the go-jose code is stripping padding from base64-encoded fields that are relevant to the JWS (e.g. the signature field) while the Boulder code is only doing so for base64-encoded fields that are relevant to ACME, like CSRs. So we can't just use one to replace the other.

There were a lot of lint failures that gofmt fixed for me and our test suite ended up passing successfully.

All of these lint failures were just import ordering failures: the import statements within each block of imports must be sorted alphabetically, so switching from "square" to "go-jose" caused the entry to move upward in the list.

@aarongable

We shut down the ACMEv1 API in summer of 2021 and no longer have use for this text. Requested by @aarongable over at #6581

jsha

Thanks for making the upgrade! There were two issues I spotted in the diff that I'd like to follow up with upstream about before we merge this. I'll file issues upstream about them.

jsha · 2023-01-12T22:15:14Z

vendor/github.com/go-jose/go-jose/v3/crypter.go

@@ -524,18 +526,18 @@ func (obj JSONWebEncryption) DecryptMulti(decryptionKey interface{}) (int, Heade
 		}
 	}

-	if plaintext == nil || err != nil {
+	if plaintext == nil {


Note for other reviewers: this was part of a lint cleanup in 7d3b437c, presumably because the err being checked here isn't the err from the loop above (which is scoped to just the loop); it's an err that was declared and checked far above here. The cleanup is correct.

jsha · 2023-01-12T22:16:43Z

vendor/github.com/go-jose/go-jose/v3/crypter.go

 		return -1, Header{}, nil, ErrCryptoFailure
 	}

 	// The "zip" header parameter may only be present in the protected header.
 	if comp := obj.protected.getCompression(); comp != "" {
-		plaintext, err = decompress(comp, plaintext)
+		plaintext, _ = decompress(comp, plaintext)


This was also part of the lint cleanup in go-jose/go-jose@7d3b437c. I'm not sure it's correct! It seems like the fix should have been to check the error, not ignore it.

jsha · 2023-01-12T22:23:32Z

vendor/github.com/go-jose/go-jose/v3/encoding.go

+// base64URLDecode is implemented as defined in https://www.rfc-editor.org/rfc/rfc7515.html#appendix-C
+func base64URLDecode(value string) ([]byte, error) {
+	value = strings.TrimRight(value, "=")
+	return base64.RawURLEncoding.DecodeString(value)
+}


This was added in go-jose/go-jose@d7b900b. I'm not sure it's correct. The linked commit cites an appendix of RFC 7515 with an example implementation, but I don't think that implementation is normative.

https://www.rfc-editor.org/rfc/rfc7515.html#section-2 has the normative definition of base64url, which does not treat the padding as optional but says "Base64 encoding using the URL- and filename-safe character set defined in Section 5 of RFC 4648 [RFC4648], with all trailing '=' characters omitted"

Agreed that treating the padding characters as optional is not correct -- if you're using base64url, they are not supposed to be there at all. But as the commit message says, they've seen it in the wild, and so have we -- we allow folks to send us CSRs with padding even though the ACME spec says they're not supposed to. So I'm not totally surprised they're being liberal in what they accept here.

aarongable · 2023-01-23T18:25:48Z

The go-jose/go-jose now has a v2.6.0 tag which matches the v2.6.0 tag we currently reference in the square/go-jose repo: https://github.com/go-jose/go-jose/releases/tag/v2.6.0

pgporada · 2023-01-23T19:58:26Z

We'll use this PR to track switching from go-jose/go-jose.v2 to go-jose/go-jose.v3. Switching from square/go-jose.v2 to go-jose/go-jose.v2 will be done at #6598.

This removes quite a bit of dependent code (295 kB on macOS). It also adds a new github.com/go-jose/go-jose.v2 , but we'll get rid of that again after letsencrypt/boulder#6581 lands. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

rhatdan · 2023-02-14T04:38:58Z

Since #6598 got merged, can this move forward?

aarongable · 2023-02-14T17:04:45Z

Yep, it can. It just hasn't been as high a priority as migrating away from the deprecated repository was.

This removes quite a bit of dependent code (295 kB on macOS). It also adds a new github.com/go-jose/go-jose.v2 , but we'll get rid of that again after letsencrypt/boulder#6581 lands. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

pgporada added 2 commits January 11, 2023 13:40

Remove square/go-jose.v2 and upgrade to go-jose/go-jose/v3

dcaad24

Remove square/go-jose.v2 and upgrade to go-jose/go-jose/v3, ran gofmt

c9a77b9

pgporada requested a review from a team as a code owner January 11, 2023 20:24

pgporada requested a review from aarongable January 11, 2023 20:24

aarongable approved these changes Jan 11, 2023

View reviewed changes

aarongable requested a review from a team January 11, 2023 22:31

pgporada mentioned this pull request Jan 12, 2023

Remove some ACMEv1 comments/documentation #6584

Merged

jsha pushed a commit that referenced this pull request Jan 12, 2023

Remove some ACMEv1 comments/documentation (#6584)

8bc4005

We shut down the ACMEv1 API in summer of 2021 and no longer have use for this text. Requested by @aarongable over at #6581

jsha requested changes Jan 12, 2023

View reviewed changes

jsha mentioned this pull request Jan 12, 2023

Base64url without padding go-jose/go-jose#18

Closed

pgporada changed the title ~~Upgrade from square/go-jose.v2 to go-jose/go-jose/v3~~ Upgrade from go/go-jose.v2 to go-jose/go-jose.v3 Jan 23, 2023

mtrmac mentioned this pull request Feb 13, 2023

Update letsencrypt/boulder after https://github.com/letsencrypt/boulder/pull/6651 containers/image#1849

Merged

beautifulentropy self-requested a review February 14, 2023 22:02

beautifulentropy approved these changes Feb 14, 2023

View reviewed changes

beautifulentropy requested a review from jsha February 14, 2023 22:02

pgporada marked this pull request as draft May 2, 2023 18:22

pgporada closed this by deleting the head repository Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade from go/go-jose.v2 to go-jose/go-jose.v3 #6581

Upgrade from go/go-jose.v2 to go-jose/go-jose.v3 #6581

pgporada commented Jan 11, 2023 •

edited

Loading

aarongable left a comment •

edited

Loading

jsha left a comment

jsha Jan 12, 2023

jsha Jan 12, 2023

jsha Jan 12, 2023

aarongable Jan 12, 2023

aarongable commented Jan 23, 2023

pgporada commented Jan 23, 2023

rhatdan commented Feb 14, 2023

aarongable commented Feb 14, 2023

	// The main reason we want to strip these ports out is so that when this header
	// is sent to the /directory endpoint we don't reply with directory URLs that
	// also contain these ports, which would then in turn end up being sent in the JWS
	// signature 'url' header, which we don't support.

Upgrade from go/go-jose.v2 to go-jose/go-jose.v3 #6581

Upgrade from go/go-jose.v2 to go-jose/go-jose.v3 #6581

Conversation

pgporada commented Jan 11, 2023 • edited Loading

aarongable left a comment • edited Loading

Choose a reason for hiding this comment

jsha left a comment

Choose a reason for hiding this comment

jsha Jan 12, 2023

Choose a reason for hiding this comment

jsha Jan 12, 2023

Choose a reason for hiding this comment

jsha Jan 12, 2023

Choose a reason for hiding this comment

aarongable Jan 12, 2023

Choose a reason for hiding this comment

aarongable commented Jan 23, 2023

pgporada commented Jan 23, 2023

rhatdan commented Feb 14, 2023

aarongable commented Feb 14, 2023

pgporada commented Jan 11, 2023 •

edited

Loading

aarongable left a comment •

edited

Loading