Remove `rusty-jwt-cli` #1670

hslatman · 2024-01-10T19:54:27Z

No description provided.

…-refactor

acme/challenge.go

…actor Wire ACME extension configuration refactor

Add OIDC token template transformation

beltram · 2024-01-16T20:37:48Z

acme/challenge.go

+		Time:   v.t,
+		Issuer: v.issuer,
+	}, 360*time.Second); err != nil {
+		return nil, nil, fmt.Errorf("failed validation: %w", err)
 	}



The access token must have a "nonce" claims that must just be present (without checking its value)

beltram · 2024-01-16T20:38:37Z

acme/challenge.go

+		Time:   v.t,
+		Issuer: v.issuer,
+	}, 360*time.Second); err != nil {
+		return nil, nil, fmt.Errorf("failed validation: %w", err)
 	}



The access token must have a "scope" claim equal to "wire_client_id"

beltram · 2024-01-16T21:04:10Z

acme/challenge.go

+		Time:   v.t,
+		Issuer: v.issuer,
+	}, 360*time.Second); err != nil {
+		return nil, nil, fmt.Errorf("failed validation: %w", err)
 	}



:Likewise an access token has a JWK in its header that must match the provided verifying key (v.tokenKey)

beltram · 2024-01-16T21:07:58Z

acme/challenge.go

+		return nil, nil, fmt.Errorf("invalid Wire DPoP token: %w", err)
+	}
+	var dpopToken wireDpopToken
+	if err := dpopJWT.Claims(v.dpopKey, &dpopToken); err != nil {


The DPoP token has a JWK in its header that must (its JWK thumbprint) be verified against the account kid (v.dpopKeyID)

beltram · 2024-01-16T21:11:37Z

acme/challenge.go

+		return nil, nil, fmt.Errorf("invalid Wire DPoP token: %w", err)
+	}
+	var dpopToken wireDpopToken
+	if err := dpopJWT.Claims(v.dpopKey, &dpopToken); err != nil {


The DPoP token must also be validated (ValidateWithLeeway) with max expiry and the same leeway

beltram · 2024-01-16T21:12:42Z

acme/challenge.go

 	}

-	return nil
+	handle, ok := dpopToken["handle"].(string)


The DPoP token must have the same ClientId (v.wireID.ClientID) as the access token

beltram · 2024-01-16T21:13:21Z

acme/challenge.go

+		return nil, nil, fmt.Errorf("invalid challenge in Wire DPoP token")
+	}
+	if challenge != v.chToken {
+		return nil, nil, fmt.Errorf("invalid Wire DPoP challenge %q", challenge)
 	}



The DPoP token must have the same "nonce" as the access token

beltram · 2024-01-16T21:13:56Z

acme/challenge.go

+		return nil, nil, fmt.Errorf("invalid challenge in Wire DPoP token")
+	}
+	if challenge != v.chToken {
+		return nil, nil, fmt.Errorf("invalid Wire DPoP challenge %q", challenge)
 	}



The DPoP token must have the same issuer as the access token

maraino · 2024-01-16T20:27:56Z

authority/provisioner/wire/oidc_options.go

+	SkipClientIDCheck          bool             `json:"-"`
+	SkipExpiryCheck            bool             `json:"-"`
+	SkipIssuerCheck            bool             `json:"-"`


What are these for? If you cannot configure them, are they something we want to keep?

Looks like they are only used for testing purposes. We can add a comment here.

Same for InsecureSkipSignatureCheck. And Now?

Agree you can remove them all

Haven't removed them yet, as they're used in some tests. The OIDC discovery work might also affect these.

acme/challenge.go

acme/db/nosql/wire.go

authority/provisioner/options.go

authority/provisioner/wire/dpop_options.go

maraino · 2024-01-16T22:24:36Z

authority/provisioner/wire/oidc_options.go

+
+type Config struct {
+	ClientID                   string           `json:"clientId,omitempty"`
+	SignatureAlgorithms        []string         `json:"signatureAlgorithms,omitempty"`


I don't think this is necessary anymore. It's also a property that is usually in the JWKs and on the id_token_signing_alg_values_supported property of the openid-configuration. And you have those in the provider.

That leaves us with only the ClientID. Assuming the rest of fields are for testing, having something like this makes more sense to me:

type OIDCOptions struct { ClientID string `json:"clientId,omitempty"` // omitempty? Provider *Provider `json:"provider,omitempty"` TransformTemplate string `json:"transform,omitempty"` }

maraino · 2024-01-16T22:37:26Z

authority/provisioner/wire/wire_options.go

+}
+
+func (o *Options) Validate() error {
+	o.validateOnce.Do(


See previous comment. Check if changing the provisioner using the admin API or HUP will reset the full provisioner and, consequently, this once, too.

I've removed this for now. I think it would work as expected with the ReloadAdminResources doing a new Init for every provisioner and replacing the provisioners, but I didn't want to block on this right now. Also, I think it should actually be part of the Init (or just after that), instead of it being triggered inside of the actual handler, so let's do it the right way (later), instead of doing it now

Improve access and dpop token validation

maraino

lgtm, but I still don't like how the OIDCOptions work. See my notes.

maraino · 2024-01-17T18:53:34Z

acme/challenge.go

+	var claims struct {
+		Name         string `json:"preferred_username,omitempty"`
+		Handle       string `json:"name"`
+		Issuer       string `json:"iss,omitempty"`
+		GivenName    string `json:"given_name,omitempty"`
+		KeyAuth      string `json:"keyauth"`
+		ACMEAudience string `json:"acme_aud,omitempty"`
+	}


omitempty only makes sense for marshaling, and it looks like we're only unmarshaling here.

Will have a look at those. There's some more places I can remove them; tests primarily

maraino · 2024-01-17T20:23:44Z

authority/provisioner/wire/oidc_options.go

+
+type Config struct {
+	ClientID            string   `json:"clientId,omitempty"`
+	SignatureAlgorithms []string `json:"signatureAlgorithms,omitempty"`


Some notes about the go-oidc implementation:

If signature algorithms are not set, then the algorithms used in the provider are used. The use case for it is to either restrict or set the algorithms if the provider doesn't have them.

The client id is matched with a string.Contains to the token audience.

As it is, the go-oidc provider will download the JWKs on each request. The provider has a mechanism to cache and re-download the keys if the keyId does not match, they don't use any cache header, this will be ok in most cases, the exception would be if the implementation uses Key IDs that are not fingerprints. It would make more sense to cache the *oidc.Provider rather than the provider config.

hslatman added 4 commits January 10, 2024 15:12

Remove the Wire CLI invocatation

29fa662

Merge branch 'wire-acme-extensions' into herman/remove-rusty-cli

033aef9

Merge branch 'wire-acme-extensions' into herman/remove-rusty-cli

a423151

Remove the rusty-jwt-cli configuration

c7892e9

github-actions bot added the needs triage Waiting for discussion / prioritization by team label Jan 10, 2024

hslatman mentioned this pull request Jan 11, 2024

Wire ACME extensions #1666

Merged

hslatman added 4 commits January 11, 2024 12:03

Merge branch 'wire-acme-extensions' into herman/remove-rusty-cli

897688a

Fix test for parseAndVerifyWireAccessToken

cd9480a

Put Wire options in lower level wire struct

acad227

Add validation of handle and token to Wire verification

b964c97

hslatman marked this pull request as ready for review January 11, 2024 13:08

hslatman requested a review from maraino January 11, 2024 13:08

hslatman added 7 commits January 11, 2024 14:24

Add verification of maximum expiry time for Wire tokens

b6fc000

Refactor the Wire option configuration

6ef64b6

Make Wire options more robust

1f5f756

Use base64 encoded signing key format

1bf807a

Add Wire DPoP proof claims verification

348363a

Remove debug err print

44721a7

Merge branch 'herman/remove-rusty-cli' into herman/wire-configuration…

7eacb68

…-refactor

maraino requested changes Jan 11, 2024

View reviewed changes

hslatman and others added 10 commits January 12, 2024 09:48

Change signature algorithm property name

79739e5

Perform initialization of DPoP and OIDC options once

2479572

Fix test; reworded error message

c8160ca

Merge pull request #1671 from smallstep/herman/wire-configuration-ref…

3f37fea

…actor Wire ACME extension configuration refactor

Change kid and dpop validation

9bb1b24

Fix linting issue

2c27e86

Add OIDC token template transformation

0ad381b

Fix Wire ID token test comment

d5b0d92

Add support for functions in OIDC token transformation template

29202ef

Store transformed OIDC token

768a089

hslatman and others added 11 commits January 15, 2024 19:46

Make the example Wire handle consistent

c46434f

Add tests for Wire ID parsing

a2304c8

Add test for wireOIDC01Validate

d84abac

Add test for wireDPOP01Validate

8f129a6

Add test case for validateWireOIDCClaims

a24b2a5

Improve test coverage for wireDPOP01Validate

7520736

Fix Wire integration test by acting on realistic access/dpop token

37106a4

Improve test coverage for wireOIDC01Validate

99934ec

Add realistic OIDC payload to Wire integration test

7680da7

Merge branch 'wire-acme-extensions' into herman/remove-rusty-cli

31bba6f

Merge pull request #1673 from smallstep/herman/wire-template-transform

17578b5

Add OIDC token template transformation

hslatman requested a review from maraino January 16, 2024 20:19

beltram suggested changes Jan 16, 2024

View reviewed changes

maraino requested changes Jan 16, 2024

View reviewed changes

hslatman and others added 9 commits January 17, 2024 00:09

Improve access and dpop token validation

0f0f060

Comment DPoP token checks that fail e2e test (currently)

0a7fe6e

Fix validations for DPoP client ID, nonce and issuer

b925474

Fix ACME Validate test for Wire DPoP challenge

f221232

Remove sync.Once for Wire configuration validation

f150a4f

Improve Wire persistence errors

36e14de

Use key authorization from ID token and handle -> preferred_username

2f3819a

Add audience validation to access, dpop and id token

19dbd02

Merge pull request #1681 from smallstep/herman/fix-wire-extensions

51d1270

Improve access and dpop token validation

hslatman requested review from maraino and beltram January 17, 2024 15:14

beltram approved these changes Jan 17, 2024

View reviewed changes

maraino approved these changes Jan 17, 2024

View reviewed changes

hslatman merged commit 7e6356e into wire-acme-extensions Jan 17, 2024
13 checks passed

hslatman deleted the herman/remove-rusty-cli branch January 17, 2024 22:14

hslatman added this to the v0.25.3 milestone Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `rusty-jwt-cli` #1670

Remove `rusty-jwt-cli` #1670

hslatman commented Jan 10, 2024

beltram Jan 16, 2024

beltram Jan 16, 2024

beltram Jan 16, 2024

beltram Jan 16, 2024

beltram Jan 16, 2024

beltram Jan 16, 2024

beltram Jan 16, 2024

beltram Jan 16, 2024

maraino Jan 16, 2024

maraino Jan 16, 2024

maraino Jan 16, 2024

beltram Jan 17, 2024

hslatman Jan 17, 2024

maraino Jan 16, 2024

maraino Jan 16, 2024

maraino Jan 16, 2024

hslatman Jan 17, 2024

maraino left a comment

maraino Jan 17, 2024

hslatman Jan 17, 2024

maraino Jan 17, 2024

Remove rusty-jwt-cli #1670

Remove rusty-jwt-cli #1670

Conversation

hslatman commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maraino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remove `rusty-jwt-cli` #1670

Remove `rusty-jwt-cli` #1670