From abd78e2d2acb5c84c4026300d88f1451c077697e Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 17 Aug 2021 13:25:55 -0700 Subject: [PATCH 1/3] Make kms uri compatible with Go 1.17. Go 1.17 introduces a change in the net/url package disallowing the use of semicolon (;) in URL queries. We used url.ParseQuery to decode the opaque string that is semicolon separated. This change replaces the semicolon with ampersands before decoding it. --- kms/uri/uri.go | 4 +++- kms/uri/uri_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/kms/uri/uri.go b/kms/uri/uri.go index 94009c47..44271e74 100644 --- a/kms/uri/uri.go +++ b/kms/uri/uri.go @@ -59,7 +59,9 @@ func Parse(rawuri string) (*URI, error) { if u.Scheme == "" { return nil, errors.Errorf("error parsing %s: scheme is missing", rawuri) } - v, err := url.ParseQuery(u.Opaque) + // Starting with Go 1.17 url.ParseQuery returns an error using semicolon as + // separator. + v, err := url.ParseQuery(strings.ReplaceAll(u.Opaque, ";", "&")) if err != nil { return nil, errors.Wrapf(err, "error parsing %s", rawuri) } diff --git a/kms/uri/uri_test.go b/kms/uri/uri_test.go index aa420db4..c2e0a9fe 100644 --- a/kms/uri/uri_test.go +++ b/kms/uri/uri_test.go @@ -274,3 +274,28 @@ func TestURI_Pin(t *testing.T) { }) } } + +func TestURI_String(t *testing.T) { + mustParse := func(s string) *URI { + u, err := Parse(s) + if err != nil { + t.Fatal(err) + } + return u + } + tests := []struct { + name string + uri *URI + want string + }{ + {"ok new", New("yubikey", url.Values{"slot-id": []string{"9a"}, "foo": []string{"bar"}}), "yubikey:foo=bar;slot-id=9a"}, + {"ok parse", mustParse("yubikey:slot-id=9a;foo=bar?bar=zar"), "yubikey:slot-id=9a;foo=bar?bar=zar"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.uri.String(); got != tt.want { + t.Errorf("URI.String() = %v, want %v", got, tt.want) + } + }) + } +} From ae58a0ee4e5291a4d55c0a3ac6400030ba6dec51 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 17 Aug 2021 16:31:53 -0700 Subject: [PATCH 2/3] Make tests compatible with Go 1.17. With Go 1.17 tls.Dial will fail if the client and server configured protocols do not overlap. See https://golang.org/doc/go1.17#ALPN --- acme/challenge.go | 6 ++++++ acme/challenge_test.go | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/acme/challenge.go b/acme/challenge.go index 1d5f0ec9..559eeb13 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -129,6 +129,12 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON conn, err := vo.TLSDial("tcp", hostPort, config) if err != nil { + // With Go 1.17+ tls.Dial fails if there's no overlap between configured + // client and server protocols. See https://golang.org/doc/go1.17#ALPN + if err.Error() == "remote error: tls: no application protocol" { + return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, + "cannot negotiate ALPN acme-tls/1 protocol for tls-alpn-01 challenge")) + } return storeError(ctx, db, ch, false, WrapError(ErrorConnectionType, err, "error doing TLS dial for %s", hostPort)) } diff --git a/acme/challenge_test.go b/acme/challenge_test.go index bb9a2507..97c5e4cd 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -1395,7 +1395,7 @@ func TestTLSALPN01Validate(t *testing.T) { assert.Equals(t, updch.Type, ch.Type) assert.Equals(t, updch.Value, ch.Value) - err := NewError(ErrorConnectionType, "error doing TLS dial for %v:443: tls: DialWithDialer timed out", ch.Value) + err := NewError(ErrorConnectionType, "error doing TLS dial for %v:443:", ch.Value) assert.HasPrefix(t, updch.Error.Err.Error(), err.Err.Error()) assert.Equals(t, updch.Error.Type, err.Type) From dc5205cc7276b8e803ffc8eb6f08cdb88b6d8cb6 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 17 Aug 2021 17:06:25 -0700 Subject: [PATCH 3/3] Extract the tls error code and fail accordingly. --- acme/challenge.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index 559eeb13..70c52578 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -10,11 +10,13 @@ import ( "encoding/base64" "encoding/hex" "encoding/json" + "errors" "fmt" "io/ioutil" "net" "net/http" "net/url" + "reflect" "strings" "time" @@ -114,6 +116,17 @@ func http01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWeb return nil } +func tlsAlert(err error) uint8 { + var opErr *net.OpError + if errors.As(err, &opErr) { + v := reflect.ValueOf(opErr.Err) + if v.Kind() == reflect.Uint8 { + return uint8(v.Uint()) + } + } + return 0 +} + func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSONWebKey, vo *ValidateChallengeOptions) error { config := &tls.Config{ NextProtos: []string{"acme-tls/1"}, @@ -130,8 +143,10 @@ func tlsalpn01Validate(ctx context.Context, ch *Challenge, db DB, jwk *jose.JSON conn, err := vo.TLSDial("tcp", hostPort, config) if err != nil { // With Go 1.17+ tls.Dial fails if there's no overlap between configured - // client and server protocols. See https://golang.org/doc/go1.17#ALPN - if err.Error() == "remote error: tls: no application protocol" { + // client and server protocols. When this happens the connection is + // closed with the error no_application_protocol(120) as required by + // RFC7301. See https://golang.org/doc/go1.17#ALPN + if tlsAlert(err) == 120 { return storeError(ctx, db, ch, true, NewError(ErrorRejectedIdentifierType, "cannot negotiate ALPN acme-tls/1 protocol for tls-alpn-01 challenge")) }