Implement OpenID Connect authentication flow

[mynona]

(Summary as requested by Vamsi)

1. Token Payload with the Access Token as exemplary case:

The scope in the payload of the AccessToken should be a String: where the scopes are separated by space delimeters:

scope: "openid email something"

public protocol AccessToken: JWTPayload {
    var jti: String { get } // this was a breaking change
    var clientID: String { get }
    var userID: String? { get }
    var scopes: [String]? { get }
    var expiryTime: Date { get }
}

Scopes should be returned as String? value with space delimeters. Example: "openid email something something"

To avoid breaking changes as vapor/oauth works also for simple OAuth2.0 flows without OpenID Connect, it would make sense to remove the protocol JWTPayload from AccessToken and create a new protocol for the JWTPayload.

This would give you the opportunity to use the correct claim names.

public protocol AccessTokenJWT: JWTPayload {

   var jti: String
   var aud: String // clientID
   var sub: String? // userID
   var scopes: String?
   var exp: Date // expiryTime
   var iss: String // issuer
   var iat: Date // issuedAT

// …

}

(And to make all non required claims based on the OpenIDConnect specification optional)

It is also possible to leave this completely to the consumer of vapor/oauth. I created the payload structs myself but the whole framework would be more streamlined if the JWT specification is part of it.

Up to you if you would see this valuable as part of the repository or if every consumer has to create the correct JWT payloads themselves. My reasoning is that some structured approach might actually be easier in terms of usability.

[mynona]

(Exactly the same applies for the IDToken and the RefreshToken with varying claims)

[mynona]

2. Scope must be validated with client scopes

Reported here: https://github.com/vamsii777/vapor-oauth/issues/11#issuecomment-1890941857

In the AccessToken we have also "aud" which represents the clientID.

Based on the userinfo endpoint specification https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse a client would for example request

"email phone address profile customized1 customized2"

The OpenID Provider should then return only the valid user attributes FOR THIS CLIENT.

At the moment we are returning all user attributes because the UserManager doesn't know anything except the userID:

func getUser(userID: String) async throws -> VaporOAuth.OAuthUser? {

If we would have an optional parameter clientID the UserManager could look up the scopes for this client (AccessToken aud) and check what parameters it should return.

What do you think about extending getUser:

getUser(userID: String, clientID: String?)

I believe it would not be good if the returned attributes are automatically managed as there are also customized scopes allowed and the OpenID specification describes that out of security reasons not all requested attributes per scope must be returned. So, this should be handled by the consumer = customized UserManager

[mynona]

3. Add the missing standard claims in OAuthUser & deviations

Specification: https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims

Deviation

• We use id instead of sub

Missing claims

• preferred_user_name
• email_verified
• phone_number_verified
• address = Object

Address object: https://openid.net/specs/openid-connect-core-1_0.html#AddressClaim

[mynona]

4. Userinfo endpoint must be protected by https://

Specification: https://openid.net/specs/openid-connect-core-1_0.html#UserInfo

token_info and userinfo must be protected by https. For token_info this is already implemented but I couldn't see if this is implemented for the new userinfo endpoint.

It is important that the code works during implementation. Therefore, you find in:

ClientValidator.swift

if environment == .production {
            if redirectURI.scheme != "https" {
                throw AuthorizationError.httpRedirectURI
            }
        }

From a security perspective it would be great if the same could be done for userinfo as we are exchanging sensitive data via this endpoint.

[mynona]

5. KeyManager

This was already documented here: https://github.com/vamsii777/vapor-oauth/discussions/5#discussioncomment-7999437

The following functions are unused at the moment:

the generate key, store key, retrieve key (?) functions are never called

I like the idea to prepare everything for key rotation and supporting multiple keys but out of the code the desired target is not obvious. Therfore, it would be great if the feature could either be extended with what is planned or at least to have some comments how those functions should be used by the consumers of vapor/oauth

As those functions are not called it is completely unclear how keyIdentifiers would be shared with the encompassing framework. At the moment there is just a call

func publicKeyIdentifier() throws -> String
func privateKeyIdentifier() throws -> String

to retrieve a private and public Key but it remains unclear how key rotation should be integrated in the overall framework from the consumer side.

[mynona]

6. Discovery Document

Specification: https://openid.net/specs/openid-connect-discovery-1_0.html

According to the specification the following claims should be optional. At the moment all claims are mandatory and it should be left to the consumer to decide which optional claims he wants to provide.

• userinfo_endpoint
• registration_endpoint
• scopes_supported
• response_modes_supported
• grant_types_supported
• acr_values_supported
• id_token_encryption_alg_values_supported
• id_token_encryption_enc_values_supported
• userinfo_signing_alg_values_supported
• userinfo_encryption_enc_values_supported
• request_object_signing_alg_values_supported
• request_object_encryption_alg_values_supported
• request_object_encryption_enc_values_supported
• token_endpoint_auth_methods_supported
• token_endpoint_auth_signing_alg_values_supported
• display_values_supported
• claim_types_supported
• claims_supported
• service_documentation
• claims_locales_supported
• ui_locales_supported
• claims_parameter_supported
• request_parameter_supported
• request_uri_parameter_supported
• require_request_uri_registration
• op_policy_uri
• op_tos_uri

The problem if all are mandatory (and there is not a full coverage of all parameters yet) is that you have to provide at least an empty String and then the value is returned but optional values you might not want to even communicate.

[mynona]

7. (Session) Cookie issue

Reported here: https://github.com/vamsii777/vapor-oauth/discussions/5#discussioncomment-7999437 and with screens here: https://github.com/vamsii777/vapor-oauth/pull/3#issuecomment-1869509640

I am not sure if this is my implementation mistake but I am deliberately sending the session cookie and unfortunately it doesn't get returned. This is a problem because I would have persisted the state and codeChallenge either as hashed cookie value or as client session. But if I don't get the sessionID back (or the cookies) then it is very hard to create for each authorization request different state and codeChallenges which is the whole point of the exercise.

Of course, there are work arounds possible but first we should check if it can be achieved as part of the Authorization Grant Flow that the session cookies are returned.

Besides, I also noted that the session cookie cannot be renamed "vapor-session". If you provide another session name there are even more issues. I would assume, the consumer might want to name the OpenID Provider session differently.

[mynona]

@vamsii777 That is all that I could observe so far. Great job done! I am really looking forward when this will be released and can be used in production :-)

[mynona]

8. client_secret validation

If the client secret is hashed in the database the following validation in the oauth repository will fail:

guard clientSecret == client.clientSecret else {
            throw ClientError.unauthorized
        }

We would need to use

func verifySecret(_ secret: String) throws -> Bool { try Bcrypt.verify(secret, created: self.clientSecret!) }

It would actually be good practise to require encrypted passwords also for the resource_server. In this case the secret validation would need to be updated.

[mynona]

9. Signing error

[ WARNING ] JWTKit error: signing algorithm error: bioConversionFailure [request-id: B6023AE8-4EEE-4FF2-A5AE-626C77A0CBAE]

Using es256 fails on the server and client examples you provided. Maybe I am doing something wrong?

func makeJWTSigner() async throws -> JWTSigner {
        let privateKeyIdentifier = try await keyManagementService.privateKeyIdentifier()
        let privateKey = try await keyManagementService.retrieveKey(identifier: privateKeyIdentifier, keyType: .private)
        return JWTSigner.es256(key: try .private(pem: privateKey))
    }