diff --git a/charts/identity/crds/identity.unikorn-cloud.org_users.yaml b/charts/identity/crds/identity.unikorn-cloud.org_users.yaml index 21c4cc2..19a579a 100644 --- a/charts/identity/crds/identity.unikorn-cloud.org_users.yaml +++ b/charts/identity/crds/identity.unikorn-cloud.org_users.yaml @@ -56,8 +56,8 @@ spec: properties: accessToken: description: |- - AccessToken s the access token currently issued for the - session. + AccessToken is the access token currently issued for the + session. This field is opaque, and may be hashed for security. type: string authorizationCodeID: description: |- @@ -74,7 +74,8 @@ spec: refreshToken: description: |- RefreshToken is the single-use refresh token currently - issued for the session. + issued for the session. This field is opaque, and may be hashed + for security. type: string required: - accessToken diff --git a/pkg/apis/unikorn/v1alpha1/helpers.go b/pkg/apis/unikorn/v1alpha1/helpers.go index d1b26f4..dab5fa1 100644 --- a/pkg/apis/unikorn/v1alpha1/helpers.go +++ b/pkg/apis/unikorn/v1alpha1/helpers.go @@ -17,8 +17,16 @@ limitations under the License. package v1alpha1 import ( + "bytes" + "crypto/pbkdf2" + "crypto/rand" + "crypto/sha512" + "encoding/base64" "errors" + "fmt" "slices" + "strconv" + "strings" unikornv1core "github.com/unikorn-cloud/core/pkg/apis/unikorn/v1alpha1" @@ -28,6 +36,10 @@ import ( var ( ErrReference = errors.New("resource reference error") + + ErrReadLength = errors.New("read length not as expected") + + ErrValidation = errors.New("validation error") ) // Paused implements the ReconcilePauser interface. @@ -66,3 +78,102 @@ func (u *User) Session(clientID string) (*UserSession, error) { return &u.Spec.Sessions[index], nil } + +const ( + // saltLength is the number of bytes of cryptographically random data + // for salted hashing. 128 bits is recommended by NIST. + // See https://en.wikipedia.org/wiki/PBKDF2 for more details. + saltLength = 16 + + // hashIterations defines the number of iterations for a salted hash + // function. The bigger the number, the harder to exhaustively search + // but also the longer it takes to compute. + // See https://en.wikipedia.org/wiki/PBKDF2 for more details. + // However... we are just hashing a JWE encoded token, not a password, + // thus it's already a millin times harder to guess than a password! + hashIterations = 1 + + // algorithmSha512 indicates the sha512 hashing algorithm. + algorithmSha512 = "sha512" +) + +// Set encodes the raw value as a PBKDF2 key. +func (t *Token) Set(value string) error { + salt := make([]byte, saltLength) + + if n, err := rand.Read(salt); err != nil { + return err + } else if n != saltLength { + return fmt.Errorf("%w: unable to create salt for token hashing", ErrReadLength) + } + + key, err := pbkdf2.Key(sha512.New, value, salt, hashIterations, sha512.Size) + if err != nil { + return err + } + + encoding := algorithmSha512 + "$" + base64.RawURLEncoding.EncodeToString(salt) + "$" + strconv.Itoa(hashIterations) + "$" + base64.RawURLEncoding.EncodeToString(key) + + *t = Token(encoding) + + return nil +} + +// Validate checks the provided value matches the hashed value in persistent +// storage. +func (t *Token) Validate(value string) error { + encoding := string(*t) + + // TODO: this aids in the transition and can be removed soon after. + // Like 3 months after (by default) given the lifetime of service + // account tokens. + if !strings.Contains(encoding, "$") { + if value != encoding { + return fmt.Errorf("%w: plantext token values do not match", ErrValidation) + } + + return nil + } + + parts := strings.Split(encoding, "$") + + if len(parts) != 4 { + return fmt.Errorf("%w: token encoding malformed", ErrValidation) + } + + algorithm := parts[0] + if algorithm != algorithmSha512 { + return fmt.Errorf("%w: unsupported hash algorithm %s", ErrValidation, algorithm) + } + + salt, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + return err + } + + iterations, err := strconv.Atoi(parts[2]) + if err != nil { + return err + } + + hash, err := base64.RawURLEncoding.DecodeString(parts[3]) + if err != nil { + return err + } + + key, err := pbkdf2.Key(sha512.New, value, salt, iterations, sha512.Size) + if err != nil { + return err + } + + if !bytes.Equal(hash, key) { + return fmt.Errorf("%w: token mismatch", ErrValidation) + } + + return nil +} + +// Clear resets a token. +func (t *Token) Clear() { + *t = Token("") +} diff --git a/pkg/apis/unikorn/v1alpha1/types.go b/pkg/apis/unikorn/v1alpha1/types.go index a4c8b44..3f50e9a 100644 --- a/pkg/apis/unikorn/v1alpha1/types.go +++ b/pkg/apis/unikorn/v1alpha1/types.go @@ -310,18 +310,22 @@ type UserSignup struct { ClientID string `json:"clientID"` } +// Token abstracts token storage in a CRD, with possible obfuscation. +type Token string + type UserSession struct { // ClientID is the client the session is bound to. ClientID string `json:"clientID"` // AuthorizationCodeID is the authorization code ID used to generate // the tokens. AuthorizationCodeID string `json:"authorizationCodeID"` - // AccessToken s the access token currently issued for the - // session. - AccessToken string `json:"accessToken"` + // AccessToken is the access token currently issued for the + // session. This field is opaque, and may be hashed for security. + AccessToken Token `json:"accessToken"` // RefreshToken is the single-use refresh token currently - // issued for the session. - RefreshToken string `json:"refreshToken"` + // issued for the session. This field is opaque, and may be hashed + // for security. + RefreshToken Token `json:"refreshToken"` // LastAuthentication records when the user last authenticated. LastAuthentication *metav1.Time `json:"lastAuthentication,omitempty"` } @@ -382,7 +386,7 @@ type ServiceAccountSpec struct { Tags unikornv1core.TagList `json:"tags,omitempty"` // AccessToken is the encrypted access token that is valid for this // service acocunt. - AccessToken string `json:"accessToken"` + AccessToken Token `json:"accessToken"` // Expiry is a hint as to when the issued token will exipre. // The access token itself is the source of truth, provided the private key is // still around, so this is a fallback, as well as a cache to improve API read diff --git a/pkg/handler/serviceaccounts/client.go b/pkg/handler/serviceaccounts/client.go index 27c0f1e..7a698bf 100644 --- a/pkg/handler/serviceaccounts/client.go +++ b/pkg/handler/serviceaccounts/client.go @@ -96,7 +96,7 @@ func convert(in *unikornv1.ServiceAccount, groups *unikornv1.GroupList) *openapi // convertCreate converts from Kubernetes into OpenAPi for create/update requests that // have extra information e.g. the access token. -func convertCreate(in *unikornv1.ServiceAccount, groups *unikornv1.GroupList) *openapi.ServiceAccountCreate { +func convertCreate(in *unikornv1.ServiceAccount, groups *unikornv1.GroupList, accessToken string) *openapi.ServiceAccountCreate { temp := convert(in, groups) out := &openapi.ServiceAccountCreate{ @@ -105,7 +105,7 @@ func convertCreate(in *unikornv1.ServiceAccount, groups *unikornv1.GroupList) *o Status: temp.Status, } - out.Status.AccessToken = &in.Spec.AccessToken + out.Status.AccessToken = &accessToken return out } @@ -163,14 +163,6 @@ func (c *Client) generate(ctx context.Context, organization *organizations.Meta, }, } - tokens, err := c.generateAccessToken(ctx, organization, out.Name) - if err != nil { - return nil, errors.OAuth2ServerError("unable to issue access token").WithError(err) - } - - out.Spec.Expiry = &metav1.Time{Time: tokens.Expiry} - out.Spec.AccessToken = tokens.AccessToken - return out, nil } @@ -246,6 +238,17 @@ func (c *Client) Create(ctx context.Context, organizationID string, request *ope return nil, errors.OAuth2ServerError("failed to generate service account").WithError(err) } + tokens, err := c.generateAccessToken(ctx, organization, resource.Name) + if err != nil { + return nil, errors.OAuth2ServerError("unable to issue access token").WithError(err) + } + + resource.Spec.Expiry = &metav1.Time{Time: tokens.Expiry} + + if err := resource.Spec.AccessToken.Set(tokens.AccessToken); err != nil { + return nil, errors.OAuth2ServerError("failed to set access token").WithError(err) + } + if err := c.client.Create(ctx, resource); err != nil { return nil, errors.OAuth2ServerError("failed to create service account").WithError(err) } @@ -259,7 +262,7 @@ func (c *Client) Create(ctx context.Context, organizationID string, request *ope return nil, err } - return convertCreate(resource, groups), nil + return convertCreate(resource, groups, tokens.AccessToken), nil } // Get retrieves information about a service account. @@ -370,7 +373,10 @@ func (c *Client) Rotate(ctx context.Context, organizationID, serviceAccountID st updated := current.DeepCopy() updated.Spec.Expiry = &metav1.Time{Time: tokens.Expiry} - updated.Spec.AccessToken = tokens.AccessToken + + if err := updated.Spec.AccessToken.Set(tokens.AccessToken); err != nil { + return nil, errors.OAuth2ServerError("failed to set access token").WithError(err) + } if err := c.client.Patch(ctx, updated, client.MergeFrom(current)); err != nil { return nil, errors.OAuth2ServerError("failed to patch group").WithError(err) @@ -383,7 +389,7 @@ func (c *Client) Rotate(ctx context.Context, organizationID, serviceAccountID st return nil, err } - return convertCreate(updated, groups), nil + return convertCreate(updated, groups, tokens.AccessToken), nil } // Delete removes the service account and revokes the access token. diff --git a/pkg/oauth2/oauth2.go b/pkg/oauth2/oauth2.go index 0774c69..dcdd83b 100644 --- a/pkg/oauth2/oauth2.go +++ b/pkg/oauth2/oauth2.go @@ -1639,15 +1639,15 @@ func (a *Authenticator) validateRefreshToken(ctx context.Context, r *http.Reques return errors.OAuth2InvalidGrant("no active session for user found") } - if user.Spec.Sessions[index].RefreshToken != refreshToken { - return errors.OAuth2InvalidGrant("refresh token reuse") + if err := user.Spec.Sessions[index].RefreshToken.Validate(refreshToken); err != nil { + return errors.OAuth2InvalidGrant("refresh token invalid or possible reuse").WithError(err) } // Things can still go wrong between here and issuing the new token, so invalidate // the session now rather than relying on the reissue doing it for us. a.InvalidateToken(ctx, user.Spec.Sessions[index].AccessToken) - user.Spec.Sessions[index].RefreshToken = "" + user.Spec.Sessions[index].RefreshToken.Clear() if err := a.client.Update(ctx, user); err != nil { return errors.OAuth2ServerError("failed to revoke user session").WithError(err) diff --git a/pkg/oauth2/tokens.go b/pkg/oauth2/tokens.go index f9a0fc0..8863b1f 100644 --- a/pkg/oauth2/tokens.go +++ b/pkg/oauth2/tokens.go @@ -164,10 +164,14 @@ func (a *Authenticator) updateSession(ctx context.Context, user *unikornv1.User, a.InvalidateToken(ctx, session.AccessToken) } - session.AccessToken = tokens.AccessToken + if err := session.AccessToken.Set(tokens.AccessToken); err != nil { + return time.Time{}, err + } if tokens.RefreshToken != nil { - session.RefreshToken = *tokens.RefreshToken + if err := session.RefreshToken.Set(*tokens.RefreshToken); err != nil { + return time.Time{}, err + } } if info.Interactive { @@ -352,8 +356,8 @@ func (a *Authenticator) verifyServiceAccount(ctx context.Context, info *VerifyIn return err } - if info.Token != serviceAccount.Spec.AccessToken { - return fmt.Errorf("%w: service account token invalid", ErrTokenVerification) + if err := serviceAccount.Spec.AccessToken.Validate(info.Token); err != nil { + return fmt.Errorf("%w: service account token invalid", err) } return nil @@ -379,8 +383,8 @@ func (a *Authenticator) verifyUserSession(ctx context.Context, info *VerifyInfo, return fmt.Errorf("%w: no active session for token", ErrTokenVerification) } - if user.Spec.Sessions[index].AccessToken != info.Token { - return fmt.Errorf("%w: token invalid for active session", ErrTokenVerification) + if err := user.Spec.Sessions[index].AccessToken.Validate(info.Token); err != nil { + return fmt.Errorf("%w: token invalid for active session", err) } return nil @@ -388,6 +392,20 @@ func (a *Authenticator) verifyUserSession(ctx context.Context, info *VerifyInfo, // InvalidateToken immediately invalidates the token so it's unusable again. // TODO: this only considers caching in the identity service, it's still usable. -func (a *Authenticator) InvalidateToken(ctx context.Context, token string) { - a.tokenCache.Remove(token) +func (a *Authenticator) InvalidateToken(ctx context.Context, token unikornv1.Token) { + // TODO: This needs a bit of a rethink, we cache the raw access token in memory + // to avoid the decryption penalty, however the token stored in persistent storage + // is hashed, thus we need to do a potentially costly exhaustive search. + for _, key := range a.tokenCache.Keys() { + t, ok := key.(string) + if !ok { + continue + } + + if err := token.Validate(t); err != nil { + continue + } + + a.tokenCache.Remove(key) + } }