feat(oidc-auth): support empty OIDC client secret

[tarasglek]

Hi, I found that this library does not work with https://lastlogin.net/developers/

Would a change like this be ok?

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 215f376d4b232b0e2f72851e2e95de9761053f22

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@hono/oidc-auth	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[yusukebe]

@tarasglek

Can you run yarn changeset on the top of the project and push the generated file?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.50%. Comparing base (6c36f52) to head (215f376). Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/oidc-auth/src/index.ts	66.66%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1126      +/-   ##
==========================================
- Coverage   79.53%   79.50%   -0.03%     
==========================================
  Files          77       77              
  Lines        2282     2284       +2     
  Branches      578      580       +2     
==========================================
+ Hits         1815     1816       +1     
- Misses        391      392       +1     
  Partials       76       76              

Flag	Coverage Δ
oidc-auth	9.98% <66.66%> (-0.02%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[yusukebe]

Hi @hnw! Can you review this? We should add tests, but if it's difficult, it's okay not to.

[tarasglek]

I ran yarn changeset, hope I did this right

[tarasglek]

all good now?

[tarasglek]

ping to get this landed

[yusukebe]

@tarasglek

Can you write a proper test?

[hnw]

@tarasglek Hi, thank you for the pull request! Sorry for the delay in reviewing. I also appreciate you reporting the specific issue regarding integration with LastLogin.

I understand that your proposal aims to support the token_endpoint_auth_method:'none' (no client secret) case. I understand that such a configuration might be necessary for providers primarily targeting Public Clients (like web frontends or mobile apps), such as LastLogin.

On the other hand, the oidc-auth middleware is currently designed and optimized for Confidential Clients, which typically run on a server and can securely store a client secret. For Confidential Clients, security best practices for OAuth 2.0 and OpenID Connect strongly recommend using client authentication methods like client_secret_basic or client_secret_post. This is crucial to prevent client impersonation. Therefore, simply adding an option to forgo client authentication (none) in a middleware intended for Confidential Clients raises security concerns for us.

Regarding this point, I would also like to investigate further, for instance by checking LastLogin's documentation for their recommended settings when used as a Confidential Client. If you happen to know of any information (like documentation) from LastLogin regarding recommended settings for Confidential Clients or security considerations when using none, sharing it would be very helpful.

We would appreciate further discussion on whether there's a safe way to support none in this middleware, or how we should consider the scope of the middleware going forward.

[tarasglek]

@tarasglek

Can you write a proper test?

I spent a few hours on this, I can't.

When you setup .env for lastpass like:

OIDC_AUTH_SECRET=cf7e368c52134dc2aa1ef4a7ecc1755b
OIDC_CLIENT_ID=https://your.app.url
OIDC_CLIENT_SECRET=
OIDC_ISSUER=https://lastlogin.net/

with

app.use('*', oidcAuthMiddleware())
app.get('/', async (c) => {
  const auth = await getAuth(c)
  console.log("auth:", auth)
  return c.html(`Hello {auth?.email}`)
})

You get

OperationProcessingError: unexpected "state" response parameter value
    at Module.validateAuthResponse (file:///home/taras/.cache/deno/npm/registry.npmjs.org/oauth4webapi/2.17.0/build/index.js:1755:23)
    at processOAuthCallback (file:///home/taras/smallweb/zdr/hono-oidc-auth.ts:371:25)
    at eventLoopTick (ext:core/01_core.js:178:7)
    at async file:///home/taras/smallweb/zdr/$deno$stdin.mts:128:30
    at async mapped (ext:deno_http/00_serve.ts:406:18) {
  name: "OperationProcessingError"
}

From oauth4webapi. I think this means I'd have to mock responses from oauth server to repro this...that's too hard for me.

@tarasglek Hi, thank you for the pull request! Sorry for the delay in reviewing. I also appreciate you reporting the specific issue regarding integration with LastLogin.

I understand that your proposal aims to support the token_endpoint_auth_method:'none' (no client secret) case. I understand that such a configuration might be necessary for providers primarily targeting Public Clients (like web frontends or mobile apps), such as LastLogin.

On the other hand, the oidc-auth middleware is currently designed and optimized for Confidential Clients, which typically run on a server and can securely store a client secret. For Confidential Clients, security best practices for OAuth 2.0 and OpenID Connect strongly recommend using client authentication methods like client_secret_basic or client_secret_post. This is crucial to prevent client impersonation. Therefore, simply adding an option to forgo client authentication (none) in a middleware intended for Confidential Clients raises security concerns for us.

Regarding this point, I would also like to investigate further, for instance by checking LastLogin's documentation for their recommended settings when used as a Confidential Client. If you happen to know of any information (like documentation) from LastLogin regarding recommended settings for Confidential Clients or security considerations when using none, sharing it would be very helpful.

We would appreciate further discussion on whether there's a safe way to support none in this middleware, or how we should consider the scope of the middleware going forward.

Their policy on this is https://lastlogin.net/developers/

I would really like to use hono oidc without a client secret, as this is the most straight-forward oidc lib I found so far.

[tarasglek]

I am a little embarrassed to note, that lastlogin does work if I put in a random non-blank client_secret. Maybe @anderspitman, could change docs to put in any value in client_secret and this is a lastlogin instruction bug.

[anderspitman]

Yeah that makes sense @tarasglek. I opened an issue for it.

@hnw re: confidential clients. LastLogin is almost entirely stateless (other than rate limiting), so I'm not really sure of a good way to persist a client_secret. Since PKCE is considered good enough for public clients, I've always considered it good enough for all clients. Obviously a secret adds defense in depth, but I don't think it's strictly necessary. If you have anything I can read that contradicts that, I'd be happy to look at it.

PS - thanks for your work on hono!