vamsii777 / vapor-oauth

OAuth2 Provider Library for Vapor
MIT License
3 stars 2 forks source link

Implement OpenID Connect #3

Open vamsii777 opened 9 months ago

vamsii777 commented 9 months ago

This WIP PR is dedicated to implementing OpenID Connect support within the Vapor OAuth library. It's a crucial step towards enhancing the library's capabilities and enabling secure authentication and user authorization through OpenID Connect protocols.

mynona commented 9 months ago

Hi, as I cannot create issues, I found one security leak with the PKCE flow:

// Optional PKCE validation
if let codeChallenge = code.codeChallenge, let codeChallengeMethod = code.codeChallengeMethod,
  let verifier = codeVerifier
{
  switch codeChallengeMethod {
  case "S256":
    // Transform the codeVerifier using SHA256 and base64-url-encode it
    guard let verifierData = verifier.data(using: .utf8) else { return false }
    let verifierHash = SHA256.hash(data: verifierData)
    let encodedVerifier = Data(verifierHash).base64URLEncodedString()

    return codeChallenge == encodedVerifier
  default:
    // If the code challenge method is unknown, fail the validation
    return false
  }
}

If only one value is not provided, then the whole PKCE challenge doesn't happen and the authorisation code is sent without code validation. So, if somebody implements it badly and does for example not send the code_verifier, the whole PKCE validation is not happening.

Wouldn't it be better to trigger an error if at least one parameter is provided? I get the point that you cannot check if the PKCE flow was started before the validation. Maybe this is better. What do you think?

Apart from this I tested it and everything worked fine – as far as I understand everything.

mynona commented 9 months ago

And what do you think about refresh-token rotation? What is your opinion?

mynona commented 8 months ago

Just to give you a heads up:

There is still the problem with the code challenge mentioned above Therefore, the code fails when two scopes are defined.

let content = OAuth_AuthorizationRequest(
  client_id: "1",
  redirect_uri: "http://localhost:8089/callback",
  state: "ping-pong",
  response_type: "code",
  scope: ["admin", "openid"],
  code_challenge: "\(codeChallenge)",
  code_challenge_method: "S256",
  nonce: nonce
)

let uri =
  "http://localhost:8090/oauth/authorize?client_id=\(content.client_id)&redirect_uri=\(content.redirect_uri)&scope=\(content.scope.joined(separator: ","))&response_type=\(content.response_type)&state=\(content.state)&code_challenge=\(content.code_challenge)&code_challenge_method=\(content.code_challenge_method)&nonce=\(nonce)"

Bildschirmfoto 2023-12-25 um 23 17 29

Bildschirmfoto 2023-12-25 um 23 24 06

Additionally I see a bug with the JSON Serialisation when you request only the scope ["openid"] for the response_type "code".

Hope this is of some help to you. If my comments annoy you just let me know and I stop :-)

vamsii777 commented 8 months ago

Regarding the code challenge, can you clarify the issue? Yet to work on the scope validation, and while token refresh-token rotation is on the roadmap, it's not an immediate priority.

mynona commented 8 months ago

I think, there is only a small change needed:

Existing code:

if let clientScopes = client?.validScopes {
                for scope in requestedScopes {
                    guard clientScopes.contains(scope) else {
                        throw ScopeError.invalid
                    }
                }
            }

Change:

mynona commented 8 months ago

something like this:

     let clientScopes = ["admin","openid","something"]
      let requestedScopes1 = "admin,openid"
      let temp = requestedScopes1.components(separatedBy: ",")

      let requestedScopesSet = Set(temp)
      let clientScopesSet = Set(clientScopes)

      if requestedScopesSet.isSubset(of: clientScopesSet) == false {
         throw ScopeError.invalid
      }
mynona commented 8 months ago

Hi, I am embarrassed. I just realize, that I gave you the wrong answer.

You didn't ask for the Scope Validation but rather about the issue with the code challenge flow and the lost session cookies.

Let me share first two screenshots and then explain why this is a problem:

Screenshot 1: OAuth Server starts the Authorization Post Request. As you can see the session cookie is still part of the request

Bildschirmfoto 2023-12-26 um 13 20 29

Screenshot 2: Client receives the response from the OAuthServer No cookies are sent

Bildschirmfoto 2023-12-26 um 13 21 16

mynona commented 8 months ago

Why is this important?

On the client side you generate the state and code_verifier and you must persist this data across requests.

There are two ways recommended (but of course, there is always a work-around possible)

1) The client creates a session himself and stores it on his backend 2) The client writes a cookie with the state and code_verifier

Both approaches don't work if the cookies are not part of the request.

So, when the redirect from the Post handler happens

return request.redirect(to: redirectURI)

it would be great if the session cookie would still be included.

Another peculiarity I discovered with the vapor-session cookie. You cannot rename it. Otherwise the Authorization of the OAuthUser fails.

I hope, my comment is understandable and of help to you.

You are doing an amazing job upgrading this repository to the latest standards :-)

vamsii777 commented 8 months ago

Let me check and get back to you.

Aquaa7 commented 8 months ago

You forgot the tenant requirement. It's essential for defining user account boundaries and configurations in OpenID Connect.

vamsii777 commented 8 months ago

You forgot the tenant requirement. It's essential for defining user account boundaries and configurations in OpenID Connect.

The tenant requirement isn't mandated by OpenID Connect specifications. It's an implementation detail, often used in multi-tenant systems. OpenID relies on identifiers.

mynona commented 4 weeks ago

Vamsi, will you continue on this pr and update to the new JWTKit?

vamsii777 commented 4 weeks ago

@mynona when JWTKit 5 becomes stable i.e release, will update to it.