Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Dileep Reddem has uploaded this change for review.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M token.go
6 files changed, 200 insertions(+), 7 deletions(-)
diff --git a/clientcredentials/clientcredentials.go b/clientcredentials/clientcredentials.go
index 7a0b9ed..8b0d39a 100644
--- a/clientcredentials/clientcredentials.go
+++ b/clientcredentials/clientcredentials.go
@@ -103,7 +103,8 @@
v[k] = p
}
- tk, err := internal.RetrieveToken(c.ctx, c.conf.ClientID, c.conf.ClientSecret, c.conf.TokenURL, v, internal.AuthStyle(c.conf.AuthStyle))
+ // TODO: Add private_key_jwt support for client_credentials grant.
+ tk, err := internal.RetrieveToken(c.ctx, c.conf.ClientID, c.conf.ClientSecret, c.conf.TokenURL, "", nil, v, internal.AuthStyle(c.conf.AuthStyle))
if err != nil {
if rErr, ok := err.(*internal.RetrieveError); ok {
return nil, (*oauth2.RetrieveError)(rErr)
diff --git a/internal/token.go b/internal/token.go
index 355c386..b4e134f 100644
--- a/internal/token.go
+++ b/internal/token.go
@@ -6,12 +6,14 @@
import (
"context"
+ "crypto/rand"
"encoding/json"
"errors"
"fmt"
"io"
"io/ioutil"
"math"
+ "math/big"
"mime"
"net/http"
"net/url"
@@ -21,6 +23,13 @@
"time"
"golang.org/x/net/context/ctxhttp"
+ "golang.org/x/oauth2/jws"
+)
+
+const (
+ // ClientAssertionType is used to denote private_key_jwt client authenticaiton method
+ // described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
+ clientAssertionType = "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"
)
// Token represents the credentials used to authorize
@@ -109,6 +118,8 @@
AuthStyleUnknown AuthStyle = 0
AuthStyleInParams AuthStyle = 1
AuthStyleInHeader AuthStyle = 2
+ // AuthStylePrivateKeyJWT denotes private_key_jwt ClientAuthentication method defined at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
+ AuthStylePrivateKeyJWT AuthStyle = 3
)
// authStyleCache is the set of tokenURLs we've successfully used via
@@ -148,6 +159,74 @@
authStyleCache.m[tokenURL] = v
}
+// generateJTI generates random/unique bytes to be passed as 'jti' argument in the token request.
+// Only used in private_key_jwt variant of Token API.
+func generateJTI() (string, error) {
+ size := 32 // arbitrary power of 2
+ bytes := make([]byte, size/2)
+ if _, err := io.ReadFull(rand.Reader, bytes); err != nil {
+ return "", err
+ }
+ // convert binary to printable ascii
+ return fmt.Sprintf("%x", big.NewInt(int64(size)).SetBytes(bytes)), nil
+}
+
+// clientAssertion prepares the client_assertion element of the private_key_jwt method as described at
+// https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
+func clientAssertion(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (url.Values, error) {
+ pk, err := ParseKey(privateKey)
+ if err != nil {
+ return nil, err
+ }
+
+ jti, err := generateJTI()
+ if err != nil {
+ return nil, err
+ }
+
+ claimSet := &jws.ClaimSet{
+ Iss: clientID,
+ Sub: clientID,
+ Aud: tokenURL,
+ Jti: jti,
+ Exp: time.Now().Add(time.Hour).Unix(),
+ Iat: time.Now().Unix(),
+ }
+
+ h := jws.Header{
+ Algorithm: "RS256",
+ Typ: "JWT",
+ KeyID: keyID,
+ }
+
+ payload, err := jws.Encode(&h, claimSet, pk)
+ if err != nil {
+ return nil, err
+ }
+ v.Set("client_assertion", payload)
+ v.Set("client_assertion_type", clientAssertionType)
+ v.Set("client_id", clientID)
+
+ return v, nil
+}
+
+// newPrivateKeyJWTTokenRequest returns a new *http.Request to retrieve a new token
+// from tokenURL using the provided clientID, privatekey, keyID and POST
+// body parameters.
+func newPrivateKeyJWTTokenRequest(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (*http.Request, error) {
+ body, err := clientAssertion(tokenURL, clientID, keyID, privateKey, v)
+ if err != nil {
+ return nil, fmt.Errorf("oauth2: cannot prepare client_assertion for token request : %v", err)
+ }
+
+ req, err := http.NewRequest("POST", tokenURL, strings.NewReader(body.Encode()))
+ if err != nil {
+ return nil, err
+ }
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ return req, nil
+}
+
// newTokenRequest returns a new *http.Request to retrieve a new token
// from tokenURL using the provided clientID, clientSecret, and POST
// body parameters.
@@ -185,7 +264,10 @@
return v2
}
-func RetrieveToken(ctx context.Context, clientID, clientSecret, tokenURL string, v url.Values, authStyle AuthStyle) (*Token, error) {
+// RetrieveToken sends API to the Token Endpoint of OAUTH Provider with necessary parameters based on the configuration.
+// It also automatically tries different style of sending credentials (clientid/clientsecret)
+// depending on the server response.
+func RetrieveToken(ctx context.Context, clientID, clientSecret, tokenURL, keyID string, privateKey []byte, v url.Values, authStyle AuthStyle) (*Token, error) {
needsAuthStyleProbe := authStyle == 0
if needsAuthStyleProbe {
if style, ok := lookupAuthStyle(tokenURL); ok {
@@ -195,7 +277,13 @@
authStyle = AuthStyleInHeader // the first way we'll try
}
}
- req, err := newTokenRequest(tokenURL, clientID, clientSecret, v, authStyle)
+ var req *http.Request
+ var err error
+ if authStyle == AuthStylePrivateKeyJWT {
+ req, err = newPrivateKeyJWTTokenRequest(tokenURL, clientID, keyID, privateKey, v)
+ } else {
+ req, err = newTokenRequest(tokenURL, clientID, clientSecret, v, authStyle)
+ }
if err != nil {
return nil, err
}
diff --git a/internal/token_test.go b/internal/token_test.go
index c54095a..fde1413 100644
--- a/internal/token_test.go
+++ b/internal/token_test.go
@@ -15,6 +15,34 @@
"testing"
)
+var testPrivateKey = []byte(`-----BEGIN RSA PRIVATE KEY-----
+MIIEowIBAAKCAQEAoZdD//51RbZp51aoyB0QFKfRDmLLRCL6lCXmz6uu78rVJEXB
+9sCFwjx+sVzH2dCsqJs1DbQnnLJ0rM4Gk4UdjWWErGx0XVuGM4BEcce7GAG8q2RT
+54/SB8Pt95oUVrfa5bKpjT9UKoLL0TGqZDbnCtf9RLK3sEbQjumHyxFwDjQiqgRL
+k+9+C4tO/q1GrT3tHANHp6X8PseAugkteuT/DIWqjX3FQsA7scH0RxtOFAvHFJ1R
+i/RPnCI+okfmmYravje6Ue5xsr8YxOH14iLr6zRQla2/Rqby8M3a4IR1ho2sIelh
+jEKsooFKeA5/xoHrAHBhff6mIFYwWhUj5K+2cwIDAQABAoIBAETwzOW0ceMeqrs0
+uUi7QYeWe3ZIRxGYXNEFBJ7YUAflQR87FcBJLigK+ECCZY9z3J4Irc9da8MKTgYF
+1j9s/Qk85ShNEy5bZHunf0wN2zAoWY7D/JogPYrrmCTZm1DOGvmByp3FBYsnh36G
+Unx0AgmZ0efT2dO+uq8mSjWkiGq+PNBoHthsVhnbZlS9jOo+F4d6KehN4ic6Xylf
+0OATkH2r0lUp28MOoKm9WhVbznpr99C9bnaxeKZbqPzLV1i7j4AwtDJjQM+nlWkA
+Klfe1ikrj9VcDt+cGjkVsBu60GSUcfCdfcFeHFnQxU2PoU6mKR04wigJsJE1rLOk
+wQmYAw0CgYEA1Ekq8RjCPJyDmpZ25hJP4u9W6kxdxzyplpu4kDJlmNpIukXk2XJV
+rl+WE84/PxGhIEpwKS4d9rUJEajhh1ywtq5ncLr3h9fP9zVhucRM5vPDhoPF3IpW
+nZGl7jEvEZSt1gN1l1HL08vtoIDJH9A4DwfQcs4k+5nsb1sBQTkp6SUCgYEAwt2r
+mFwsdXWp17OIkTvAPXW+ZxEKMNm8LqbWO0DZOv9FAmYqroax59FfVqcknp/zirmz
+8UnybMW2q42epGGlO2p1XyVpFYwiyr17A0zRNS91SOfU2URZupUdxzc0uLNmw81i
+FXjIxCyd+OUQuvj0AM9+W/ja8BPq/jasBB88ybcCgYBZGmz/zZiZwEgs9sLpRv5p
+DnS421zxZ7D3Gl2ZiM3EHrswFG4+JxN3oX9oyMUbP9cVqqjxX/4Ls2kfFHe1TV6P
+Dx0z4AZN6nPMG4ftZUuyFNcY+u5t51L7yEqRc+uBwZpFniYZYafgOlR/bg79X3Ro
+OqtvxL8ZLD5lbxlTux6wUQKBgQCKR5c/FRmPeKG/qW0d8pKmimlE5jifFcOOL2IS
+xh/g18h2vV1IX9jbMh4/dXhtAABozwK8FMdtHJhWALc02v3PvenwTLHXUoxV5kPm
+wZor7bOCutC2JOSvnKV2+tv0vYoNJC+YIRAG9cpcFuDabVNpR9TZGyzx9JBrOQy0
+GB6g3QKBgFutTJZrTFMNOwvQYUhG21/EOYz6pZPCofTNBUOQM7KvbKH/UJ5kPtYW
+oK6yzHhpOGsZvly5CPau6djDhQZvzdDl2t3eRKuqTtHARhtyL5Qb7OICEsOCYVWh
+IHJQ79Wak98HWO9Dkww1mRumvEtOTqjtzTLp2w1IaKGdI+fLD3/v
+-----END RSA PRIVATE KEY-----`)
+
func TestRetrieveToken_InParams(t *testing.T) {
ResetAuthCache()
const clientID = "client-id"
@@ -29,7 +57,7 @@
io.WriteString(w, `{"access_token": "ACCESS_TOKEN", "token_type": "bearer"}`)
}))
defer ts.Close()
- _, err := RetrieveToken(context.Background(), clientID, "", ts.URL, url.Values{}, AuthStyleInParams)
+ _, err := RetrieveToken(context.Background(), clientID, "", ts.URL, "", nil, url.Values{}, AuthStyleInParams)
if err != nil {
t.Errorf("RetrieveToken = %v; want no error", err)
}
@@ -45,7 +73,7 @@
}))
defer ts.Close()
- _, err := RetrieveToken(context.Background(), clientID, "", ts.URL, url.Values{}, AuthStyleUnknown)
+ _, err := RetrieveToken(context.Background(), clientID, "", ts.URL, "", nil, url.Values{}, AuthStyleUnknown)
if err != nil {
t.Errorf("RetrieveToken (with background context) = %v; want no error", err)
}
@@ -58,7 +86,7 @@
ctx, cancel := context.WithCancel(context.Background())
cancel()
- _, err = RetrieveToken(ctx, clientID, "", cancellingts.URL, url.Values{}, AuthStyleUnknown)
+ _, err = RetrieveToken(ctx, clientID, "", cancellingts.URL, "", nil, url.Values{}, AuthStyleUnknown)
close(retrieved)
if err == nil {
t.Errorf("RetrieveToken (with cancelled context) = nil; want error")
@@ -75,3 +103,48 @@
t.Errorf("expiration time = %v; want %v", e, want)
}
}
+
+func TestRetrieveToken_PrivateKeyJWT(t *testing.T) {
+ ResetAuthCache()
+ clientID := "client-id"
+ clientAssertionType := "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"
+
+ testCases := []struct {
+ name string
+ clientID string
+ privateKey []byte
+ wantErr bool
+ }{
+ {
+ name: "Success case",
+ clientID: clientID,
+ privateKey: testPrivateKey,
+ },
+ {
+ name: "invalid private key",
+ clientID: clientID,
+ privateKey: []byte(`invalid certificate data`),
+ wantErr: true,
+ },
+ }
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ if got, want := r.FormValue("client_id"), tc.clientID; got != want {
+ t.Errorf("client_id = %q; want %q", got, want)
+ }
+ if got, want := r.FormValue("client_assertion_type"), clientAssertionType; got != want {
+ t.Errorf("client_assertion_type = %q; want %q", got, want)
+ }
+ w.Header().Set("Content-Type", "application/json")
+ io.WriteString(w, `{"access_token": "ACCESS_TOKEN", "token_type": "bearer"}`)
+ }))
+ _, err := RetrieveToken(context.Background(), tc.clientID, "", ts.URL, "", tc.privateKey, url.Values{}, AuthStylePrivateKeyJWT)
+ if err != nil && !tc.wantErr {
+ t.Errorf("RetrieveToken = %v; want no error", err)
+ }
+ ts.Close()
+ })
+ }
+
+}
diff --git a/jws/jws.go b/jws/jws.go
index 9501564..6dbf1f4 100644
--- a/jws/jws.go
+++ b/jws/jws.go
@@ -38,6 +38,7 @@
Exp int64 `json:"exp"` // the expiration time of the assertion (seconds since Unix epoch)
Iat int64 `json:"iat"` // the time the assertion was issued (seconds since Unix epoch)
Typ string `json:"typ,omitempty"` // token type (Optional).
+ Jti string `json:"jti,omitempty"` // jti denotes unique id for the token request. Used in private_key_jwt
// Email for which the application is requesting delegated access (Optional).
Sub string `json:"sub,omitempty"`
diff --git a/oauth2.go b/oauth2.go
index 291df5c..098ac72 100644
--- a/oauth2.go
+++ b/oauth2.go
@@ -57,6 +57,15 @@
// Scope specifies optional requested permissions.
Scopes []string
+
+ // Private key contains the base64 encoded contents of the RSA private key.
+ // This key would be used in Token request (https://openid.net/specs/openid-connect-core-1_0.html#TokenEndpoint).
+ // Specifically, this certificate would be used in private_key_jwt variant of client authentication method
+ // as described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
+ PrivateKey []byte
+
+ // KeyID is a hint that uniquely identfies the 'PrivateKey' used to sign jwt.
+ KeyID string
}
// A TokenSource is anything that can return a token.
@@ -97,6 +106,11 @@
// using HTTP Basic Authorization. This is an optional style
// described in the OAuth2 RFC 6749 section 2.3.1.
AuthStyleInHeader AuthStyle = 2
+
+ // AuthStylePrivateKeyJWT implies that the token request must contain a JWT instead of client_secret
+ // either in header or in post body. This is more secure way of sending the API.
+ // See https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
+ AuthStylePrivateKeyJWT AuthStyle = 3
)
var (
diff --git a/token.go b/token.go
index 8227203..c35984a 100644
--- a/token.go
+++ b/token.go
@@ -154,7 +154,7 @@
// This token is then mapped from *internal.Token into an *oauth2.Token which is returned along
// with an error..
func retrieveToken(ctx context.Context, c *Config, v url.Values) (*Token, error) {
- tk, err := internal.RetrieveToken(ctx, c.ClientID, c.ClientSecret, c.Endpoint.TokenURL, v, internal.AuthStyle(c.Endpoint.AuthStyle))
+ tk, err := internal.RetrieveToken(ctx, c.ClientID, c.ClientSecret, c.Endpoint.TokenURL, c.KeyID, c.PrivateKey, v, internal.AuthStyle(c.Endpoint.AuthStyle))
if err != nil {
if rErr, ok := err.(*internal.RetrieveError); ok {
return nil, (*RetrieveError)(rErr)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem.
10 comments:
Patchset:
What's the rationale for adding this? Is this method required by something?
This adds new API surface to a golang.org/x package, so it requires going through the Go proposal process:
https://github.com/golang/proposal
Basically: File an issue at go.dev/issue describing the new API and providing a rationale for adding it.
File clientcredentials/clientcredentials.go:
Patch Set #1, Line 106: // TODO: Add private_key_jwt support for client_credentials grant.
At a minimum this needs to return an error if AuthStylePrivateKeyJIT is used, but it should support this auth mode here unless there's a compelling reason not to.
File internal/token.go:
Patch Set #1, Line 165: size := 32 // arbitrary power of 2
Why does this need to be a power of 2?
Patch Set #1, Line 166: bytes := make([]byte, size/2)
Why size/2?
Patch Set #1, Line 171: return fmt.Sprintf("%x", big.NewInt(int64(size)).SetBytes(bytes)), nil
Just `fmt.Sprintf("%x", bytes)` will do, and avoids pulling in math/big.
Patch Set #1, Line 176: func clientAssertion(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (url.Values, error) {
Why does this return a url.Values?
Patch Set #1, Line 216: func newPrivateKeyJWTTokenRequest(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (*http.Request, error) {
This function overlaps with newTokenRequest, and is inconsistent with it:
Having separate functions might make sense, but the Values cloning behavior should be aligned.
File oauth2.go:
Patch Set #1, Line 61: // Private key contains the base64 encoded contents of the RSA private key.
This seems like it should be an `*rsa.PrivateKey`, but I see that this matches jwt.Config.PrivateKey.
Patch Set #1, Line 68: KeyID string
There probably should be an error for these fields mismatching the auth style: If you set PrivateKey or KeyID, then you must use AuthStylePrivateKeyJWT, and vice-versa.
Patch Set #1, Line 99: AuthStyleAutoDetect AuthStyle = 0
Should AutoDetect select private_key_jwt if PrivateKey is set?
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem.
Dileep Reddem uploaded patch set #2 to this change.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
8 files changed, 287 insertions(+), 7 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
8 comments:
File clientcredentials/clientcredentials.go:
Patch Set #1, Line 106: // TODO: Add private_key_jwt support for client_credentials grant.
At a minimum this needs to return an error if AuthStylePrivateKeyJIT is used, but it should support […]
Done
File internal/token.go:
Patch Set #1, Line 165: size := 32 // arbitrary power of 2
Why does this need to be a power of 2?
It's a random choice. 32 is chosen for sufficiently large size. Power of 2 as powers-of-2 are quite common.
Patch Set #1, Line 166: bytes := make([]byte, size/2)
Why size/2?
Below I was converting single-byte potentially non-printable char to a printable ascii/hex.. ie 1 byte will be converted to 2 hex chars. Hence /2.
Patch Set #1, Line 171: return fmt.Sprintf("%x", big.NewInt(int64(size)).SetBytes(bytes)), nil
Just `fmt.Sprintf("%x", bytes)` will do, and avoids pulling in math/big.
Done
Patch Set #1, Line 176: func clientAssertion(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (url.Values, error) {
Why does this return a url. […]
This returns url.Values to keep the code similar to newTokenRequest(), specifically the line that posts the API:
```
req, err := http.NewRequest("POST", tokenURL, strings.NewReader(body.Encode()))
```
I was viewing this API as a helper function that gets the data required for 'http.NewRequest(...)' similar to the if-block in lines 239-247.
Patch Set #1, Line 216: func newPrivateKeyJWTTokenRequest(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (*http.Request, error) {
This function overlaps with newTokenRequest, and is inconsistent with it: […]
Done
File oauth2.go:
Patch Set #1, Line 68: KeyID string
There probably should be an error for these fields mismatching the auth style: If you set PrivateKey […]
Done
Patch Set #1, Line 99: AuthStyleAutoDetect AuthStyle = 0
Should AutoDetect select private_key_jwt if PrivateKey is set?
Good question. Looks like this is used only by google. From the comment, it feels like this is related to AuthStyleInHeader vs AuthStyleInParam. I don't see any use cases for this. We can do it as a follow up if required. LMK.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
2 comments:
Patchset:
What's the rationale for adding this? Is this method required by something? […]
Patchset:
filed a proposal for this change - https://github.com/golang/go/issues/57186
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem.
5 comments:
File clientcredentials/clientcredentials.go:
Patch Set #1, Line 106: // TODO: Add private_key_jwt support for client_credentials grant.
Done
What's the reason for not supporting private_key_jwt here?
File internal/token.go:
Patch Set #1, Line 165: size := 32 // arbitrary power of 2
It's a random choice. 32 is chosen for sufficiently large size. […]
The comment here says "arbitrary power of 2", which implies that this needs to be a power of 2.
If it doesn't need to be a power of 2, then drop or change the comment.
Patch Set #1, Line 166: bytes := make([]byte, size/2)
Below I was converting single-byte potentially non-printable char to a printable ascii/hex.. […]
Defining 'size' as the size in hex-encoded bytes makes it more difficult to see how much entropy is present in the JWT ID. Start with the size in bytes (size = 16).
This means that if the encoding here were changed to something more compact than hex (base64, for example), the definition of the JWT ID size would not need to change.
Patch Set #1, Line 176: func clientAssertion(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (url.Values, error) {
This returns url. […]
But the url.Values is just the one passed in as a parameter. Why is it being returned? The caller already has it.
Patch Set #1, Line 216: func newPrivateKeyJWTTokenRequest(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (*http.Request, error) {
Done
Doesn't seem to be done? newPrivateKeyJWTTokenRequest doesn't clone the provided url.Values, and newTokenRequest does. Both functions should behave the same.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem.
Dileep Reddem uploaded patch set #3 to this change.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
8 files changed, 288 insertions(+), 7 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
6 comments:
Patchset:
filed https://github. […]
Done
File clientcredentials/clientcredentials.go:
Patch Set #1, Line 106: // TODO: Add private_key_jwt support for client_credentials grant.
What's the reason for not supporting private_key_jwt here?
No specific reason other than keeping the CL short. I will incorporate the calls (and add test cases) once current CL is done.
For now, we have unit tests that fail if this method is specified (as per previous review comment).
File internal/token.go:
Patch Set #1, Line 165: size := 32 // arbitrary power of 2
The comment here says "arbitrary power of 2", which implies that this needs to be a power of 2. […]
I will change the comment. It can be any size
Patch Set #1, Line 166: bytes := make([]byte, size/2)
Defining 'size' as the size in hex-encoded bytes makes it more difficult to see how much entropy is […]
got it. I made size as 32, so that current JTI will be 64. I did not want to make it 16 just in case we decide to go with other encoding like base64url
Patch Set #1, Line 176: func clientAssertion(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (url.Values, error) {
But the url.Values is just the one passed in as a parameter. […]
3 new url-values are added to this. Hence we are also returning the result.
Caller was closing the values before the call. I think that's a bit confusing. I now moved cloning of urlValues to this function before using 'v'. Let me know how does it look now.
Patch Set #1, Line 216: func newPrivateKeyJWTTokenRequest(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (*http.Request, error) {
Doesn't seem to be done? newPrivateKeyJWTTokenRequest doesn't clone the provided url. […]
In the prev snapshot, cloning is done in the call to 'clientAssertion()'. I am viewing clientAssertion() as a helper/inline.
That is, I don't see a node to clone in newPrivateKeyJWT..() where the actual use is within clientAssertion(). Now I am doing clone in clientAssertion() where it's being used.
Please check and let me know if you have suggestions.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
Dileep Reddem uploaded patch set #4 to this change.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
8 files changed, 289 insertions(+), 7 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem.
16 comments:
Patchset:
Done
The proposal needs to include the actual API being proposed. Several people on that issue have asked for it, but you haven't responded to them yet.
File clientcredentials/clientcredentials.go:
Patch Set #1, Line 106: // TODO: Add private_key_jwt support for client_credentials grant.
No specific reason other than keeping the CL short. […]
I believe that finishing support here will be a prerequisite for landing this feature; we don't want to add it half-finished and incomplete.
File internal/token.go:
Patch Set #1, Line 216: func newPrivateKeyJWTTokenRequest(tokenURL, clientID, keyID string, privateKey []byte, v url.Values) (*http.Request, error) {
In the prev snapshot, cloning is done in the call to 'clientAssertion()'. […]
Ack
File internal/token.go:
Patch Set #4, Line 119: // AuthStylePrivateKeyJWT denotes private_key_jwt ClientAuthentication method defined at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
No need for this comment here; the documentation on oauth2.AuthStylePrivateKeyJWT (which this is a copy of) is sufficient.
Patch Set #4, Line 169: return fmt.Sprintf("%x", bytes), nil
`base64.RawURLEncoding.EncodeToString(bytes)` would be more compact.
You can simplify this by dropping `newPivateKeyJWTTokenRequest` and changing this to:
```
var body url.Values
switch c.AuthStyle {
case AuthStyleInHeader:
body = v
case AuthStyleInParams:
body = cloneURLValues(v)
if c.ClientID != "" {
body.Set("client_id", c.ClientID)
}
if clientSecret != "" {
body.Set("client_secret", c.ClientSecret)
}
case AuthStylePrivateKeyJWT:
var err error
body, err = clientAssertion(c.URL, c.ClientID, c.KeyID, c.PrivateKey, v)
if err != nil {
return err
}
default:
return fmt.Errorf("oauth2: unsupported AuthStyle %v", c.AuthStyle)
}
```
This centralizes the decision of how to construct the request for various auth styles in `newTokenRequest`, rather than spreading it across two different functions.
(I'm assuming here that we add a TokenConfig type, see next comment below.)
Patch Set #4, Line 269: func RetrieveToken(ctx context.Context, clientID, clientSecret, tokenURL, keyID string, privateKey []byte, v url.Values, authStyle AuthStyle) (*Token, error) {
This parameter list is getting very long, and too many of the parameters have the same type. Let's bundle up the token configuration into a struct so we can pass it around more easily:
```
type TokenConfig struct {
AuthStyle AuthStyle
URL string
ClientID string
ClientSecret string
KeyID string
PrivateKey []byte
}
func RetrieveToken(ctx context.Context, v url.Values, c TokenConfig) (*Token, error) {
// ...
}
```File jws/jws.go:
Patch Set #4, Line 41: jti denotes unique id for the token request. Used in private_key_jwt
```
// unique token identifier, used by private_key_jwt
```
File oauth2.go:
Patch Set #1, Line 99: AuthStyleAutoDetect AuthStyle = 0
Good question. Looks like this is used only by google. […]
Leaving AutoDetect to apply only to InParams/InHeader seems fine.
The doc comment here should be updated to indicate that it only applies to those styles.
File oauth2.go:
Patch Set #4, Line 64: // as described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
```
// PrivateKey contains the contents of an RSA private key or the
// contents of a PEM file that contains a private key.
// The provided private key is used to sign JWTs when using AuthStylePrivateKeyJWT.
// PEM containers with a passphrase are not supported.
```
(This matches the language in jwt.Config.)
However, see next comment.
Patch Set #4, Line 65: PrivateKey []byte
As currently implemented, this private key is reparsed on every use.
Using a `[]byte` here matches the `jwt.Config.PrivateKey` field, but the `jwt` package parses its key only once and retains the parsed `*rsa.PrivateKey` for future use.
Either this needs to be a `*rsa.PrivateKey`, or the implementation needs to ensure somehow that it doesn't repeatedly reparse the key.
Patch Set #4, Line 67: // KeyID is a hint that uniquely identfies the 'PrivateKey' used to sign jwt.
```
// PrivateKeyID is an optional hint indicating which key is being used.
```
(Better matches the language in jwt.Config.)
(Obsolete if changed: "identifies" is typoed, and we don't generally use 'quotes' around symbol names in doc comments.)
Patch Set #4, Line 68: KeyID string
Name this PrivateKeyID, for consistency with the same field in jwt.Config.
Patch Set #4, Line 112: // See https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
```
// AuthStylePrivateKeyJWT authenticates with a JWT signed with a private key.
// The PrivateKey field must be set in the Config.
//
// This is the private_key_jwt authentication method described at:
// https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
```
Patch Set #4, Line 153: func (c *Config) Validate() error {
This change doesn't seem to require adding an exported Validate method. Unless there's a compelling need for it, make it unexported to avoid complicating the proposal?
Patch Set #4, Line 157: return errors.New("PrivateKey and KeyID must both be specified or omitted.")
When using private_key_jwt, PrivateKey must always be specified, no?
And is PrivateKeyID required? jwt.Config.PrivateKeyID seems to be the same field, and documents it as being optional.
This should also verify that the user isn't setting configuration settings that will be ignored: It should be an error to set ClientSecret when using private_key_jwt.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem.
Patch set 4:Run-TryBot +1
Attention is currently required from: Brad Fitzpatrick, Damien Neil, adam strickland.
1 comment:
Patchset:
Heads up, I'm trying to move this along in the issue, and would be happy to take this on need be. […]
Hi Adam! That would be awesome. Please go ahead!
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Dileep Reddem.
2 comments:
Patchset:
The proposal needs to include the actual API being proposed. […]
Heads up, I'm trying to move this along in the issue, and would be happy to take this on need be. Added some context [here](https://github.com/golang/go/issues/57186#issuecomment-1487852710)
Patchset:
Added a note to one of Damien's comments
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
Dileep Reddem uploaded patch set #5 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Matthew Hickford, TryBot-Result+1 by Gopher Robot
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
8 files changed, 352 insertions(+), 28 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
Dileep Reddem uploaded patch set #6 to this change.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
8 files changed, 375 insertions(+), 28 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
13 comments:
File internal/token.go:
Patch Set #4, Line 119: // AuthStylePrivateKeyJWT denotes private_key_jwt ClientAuthentication method defined at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
No need for this comment here; the documentation on oauth2. […]
Done
Patch Set #4, Line 169: return fmt.Sprintf("%x", bytes), nil
`base64.RawURLEncoding.EncodeToString(bytes)` would be more compact.
Done. Changed to the suggested API. PTAL.
You can simplify this by dropping `newPivateKeyJWTTokenRequest` and changing this to: […]
Done
Patch Set #4, Line 269: func RetrieveToken(ctx context.Context, clientID, clientSecret, tokenURL, keyID string, privateKey []byte, v url.Values, authStyle AuthStyle) (*Token, error) {
This parameter list is getting very long, and too many of the parameters have the same type. […]
Done
File jws/jws.go:
Patch Set #4, Line 41: jti denotes unique id for the token request. Used in private_key_jwt
``` […]
Done
File oauth2.go:
Patch Set #1, Line 99: AuthStyleAutoDetect AuthStyle = 0
Leaving AutoDetect to apply only to InParams/InHeader seems fine. […]
Changed the comment. PTAL
File oauth2.go:
``` […]
Okay, will change this to match with jwt.Config
Patch Set #4, Line 65: PrivateKey []byte
As currently implemented, this private key is reparsed on every use. […]
May be I'm missing something. I cannot find where the `jwt` module parses and stores it. I see that it uses `ParseKey` from internal/oauth2.go but don't see where it's stored. I agree that storing it for subsequent uses is good. But, how do determine if the key is changed?
Patch Set #4, Line 67: // KeyID is a hint that uniquely identfies the 'PrivateKey' used to sign jwt.
``` […]
Done
Patch Set #4, Line 68: KeyID string
Name this PrivateKeyID, for consistency with the same field in jwt.Config.
Done
``` […]
Done
Patch Set #4, Line 153: func (c *Config) Validate() error {
This change doesn't seem to require adding an exported Validate method. […]
Sure. We could make it internal.
Patch Set #4, Line 157: return errors.New("PrivateKey and KeyID must both be specified or omitted.")
When using private_key_jwt, PrivateKey must always be specified, no? […]
updated validate() to handle these cases.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
Dileep Reddem uploaded patch set #7 to this change.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
8 files changed, 552 insertions(+), 28 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
1 comment:
File clientcredentials/clientcredentials.go:
Patch Set #1, Line 106: // TODO: Add private_key_jwt support for client_credentials grant.
I believe that finishing support here will be a prerequisite for landing this feature; we don't want […]
Sure. Added support for that. PTAL.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
Dileep Reddem uploaded patch set #8 to this change.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
8 files changed, 631 insertions(+), 28 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
2 comments:
Patchset:
Hi Adam! That would be awesome. […]
Any update on this one?
File oauth2.go:
Patch Set #4, Line 65: PrivateKey []byte
May be I'm missing something. I cannot find where the `jwt` module parses and stores it. […]
Added a cache. Right now it is a map. So, it can grow to arbitrary size. It's small (string -> pointer). But, might be a good thing to do pruning in the future. Or we could just hold one item only. Holding one item might not work if multiple callers are involved.
PTAL and let me know if we should implement only one key or if we should restrict to any max items.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Damien Neil, Dileep Reddem, Matthew Hickford, adam strickland.
2 comments:
File clientcredentials/clientcredentials.go:
Patch Set #8, Line 58: // PrivateKeyID is an optional hint indicating which key is being used.
What form is this string in?
// PrivateKey contains the contents of an RSA private key or the
// contents of a PEM file that contains a private key. The provided
// private key is used to sign the Token request (https://openid.net/specs/openid-connect-core-1_0.html#TokenEndpoint).
// PEM containers with a passphrase are not supported.
PrivateKey []byte
// PrivateKeyID is an optional hint indicating which key is being used.
PrivateKeyID string
The "Config" type docs above say this type "describes a 2-legged OAuth2 flow".
These fields aren't part of a 2-legged OAuth2 flow, right?
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
2 comments:
File clientcredentials/clientcredentials.go:
Patch Set #8, Line 58: // PrivateKeyID is an optional hint indicating which key is being used.
What form is this string in?
KeyId is a string. This is from the JWKS spec - https://www.rfc-editor.org/rfc/rfc7517#section-4.5
// PrivateKey contains the contents of an RSA private key or the
// contents of a PEM file that contains a private key. The provided
// private key is used to sign the Token request (https://openid.net/specs/openid-connect-core-1_0.html#TokenEndpoint).
// PEM containers with a passphrase are not supported.
PrivateKey []byte
// PrivateKeyID is an optional hint indicating which key is being used.
PrivateKeyID string
The "Config" type docs above say this type "describes a 2-legged OAuth2 flow". […]
These are applicable in the 2-Legged OAuth flow as well. This can be used in lieu of client_secret when sending request for client_credentials grant. This is a more secure option. Essentially, this is a way to 'authenticate'/verify the client's identity.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem, Matthew Hickford, adam strickland.
2 comments:
File oauth2.go:
Patch Set #4, Line 65: PrivateKey []byte
Added a cache. Right now it is a map. So, it can grow to arbitrary size. […]
As you point out, using a map has issues with size growth over time. We could attempt to address that in one way or another, but this seems like unnecessary complexity. I really think this should take the parsed key as input, or possibly a crypto.Signer.
Commented on https://go.dev/issue/57186; I think that issue has more visibility, so let's work out the API there. I think the API seems fine, with the exception of the private key/signer.
File oauth2.go:
Patch Set #8, Line 68: PrivateKeyID string
The doc comment says optional, but the validate function below requires this to be set. Which is it?
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem, Matthew Hickford, adam strickland.
Dileep Reddem uploaded patch set #9 to this change.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M endpoints/endpoints.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
9 files changed, 638 insertions(+), 30 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Dileep Reddem, Matthew Hickford, adam strickland.
Dileep Reddem uploaded patch set #10 to this change.
x/oauth2: Adding support for private_key_jwt clientAuthentication method.
Details:
OAuth2 spec describes a secure way of sending token API by using private_key_jwt. This method avoids sending client_secret, and hence is extremely secure.
This is described at https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication
private_key_jwt is defined in https://datatracker.ietf.org/doc/html/rfc7523
Change-Id: I4c289a28b624990c6558fb72314a193ff28f1071
---
M clientcredentials/clientcredentials.go
M clientcredentials/clientcredentials_test.go
M endpoints/endpoints.go
M internal/token.go
M internal/token_test.go
M jws/jws.go
M oauth2.go
M oauth2_test.go
M token.go
9 files changed, 638 insertions(+), 30 deletions(-)
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
1 comment:
File oauth2.go:
Patch Set #8, Line 68: PrivateKeyID string
The doc comment says optional, but the validate function below requires this to be set. […]
Done.
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Matthew Hickford, adam strickland.
1 comment:
Patchset:
Any update on this one?
let's discuss at https://go.dev/issue/57186
To view, visit change 435795. To unsubscribe, or for help writing mail filters, visit settings.
| Hold | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |