Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: PKCE #35

Merged
merged 50 commits into from
May 15, 2023
Merged

feat: PKCE #35

merged 50 commits into from
May 15, 2023

Conversation

raynerljm
Copy link
Contributor

@raynerljm raynerljm commented Apr 19, 2023

Problem

Supporting the [Proof Key for Code Exchange (PKCE)](https://www.rfc-editor.org/rfc/rfc7636) extension to OAuth 2.0 is a current best practice for preventing authorisation code interception attacks in native apps. As per [RFC 8252 (OAuth 2.0 for Native Apps)](https://www.rfc-editor.org/rfc/rfc8252):

Public native app clients MUST implement the Proof Key for Code Exchange (PKCE [RFC7636]) extension to OAuth, and authorization servers MUST support PKCE for such clients, for the reasons detailed in Section 8.1.

Since it prevents auth code injection attacks, it is useful for client types other than native apps as well.

This PR is linked to this issue

Solution

High-level principles

As this is a relatively large PR which involves breaking changes, I would like to highlight a few high-level principles that were kept in mind during development and which should be achieved through the code changes.

  1. The SDK should be dummy

This is in line with what we have been doing so far where we would like to keep the SDK dummy. This is so that if we would like to make any changes (e.g. with validation), we are able to modify our servers directly instead of updating our SDK (as not all RPs will update their SDKs).

This is achieved by not validating the ABNF of the codeChallenge and codeVerifier received in authorizationUrl and callback respectively. (Especially because validation bugs are easier to commit and harder to rectify)

  1. We should be mindful of the exports we are providing

As we are maintaining a public SDK for people to use, we need to be very aware of what exactly we are exposing for developers to import. If we export function a, we have to continue maintaining a as an export in future versions as developers might be relying on them.

This is achieved by refactoring the SgidClient class into it's own file and keeping index.ts solely as a tunnel for exports. Additionally, we are now exporting the SDK types so developers with TypeScript can make use of them.

  1. We should be mindful of the SDK function signature

With this PR, the API of the SDK is changing (i.e. the function signature is different). Specifically, the functions now take in an instead of sequential parameters.

This is because as the list of parameters gets longer this (i) makes it harder to remember which input corresponds to which parameter and (ii) makes it very confusing when there are optional parameters - users would have to enter undefined for the optional parameters they do not want to fill in just to reach the parameter they have to fill in.

Features:

PKCE related changes

  • Added the generatePkcePair, generateCodeVerifier and generateCodeChallenge functions based on openid-client generator functions in the generators.ts file
  • Changed the authorizationUrl function params to take in an object
    • Added the codeChallenge and codeChallengeMethod parameter
  • Changed the callback function params to take in an object
    • Added the codeVerifier parameter
  • Added relevant tests for new functions
    • Added tests for generatePkcePair, generateCodeVerifier and generateCodeChallenge functions
    • Updated tests for authorizationUrl and callback

SDK improvement related changes

  • Changed the userinfo function params to take in an object
  • Changed default scope from openid myinfo.nric_number to openid myinfo.name
  • Refactored main SgidClient class into its own file sgidClient.ts and extracted types into types.ts
  • Exported sgidClient.ts, types.ts, generators.ts and util.ts through index.ts (to be explicit about what we want exported)
  • Removed apiVersion as a parameter for the constructor and use a hardcoded API_VERSION constant
  • Updated the issuer to have the v2 appended (in the constructor)
  • Turn off Remove eslint-no-throw-sync-func ESLint rule

README related changes

  • Added description + example usage for generatePkcePair
  • Added function signature (parameters + return type) indicating the type, whether it's optional, the default value, and a brief explanation
  • Removed the code snippet inline comments to the function signature

Non-implemented changes
- Changing the constructor param from redirectUri to redirectUris

Tests

  • Added test suites for generatePkcePair, generateCodeVerifier, and generateCodeChallenge
  • Updated test cases for authorizationUrl and callback functions
  • Tested E2E locally following this flow

Verified that all tests are passing
image

@raynerljm raynerljm changed the title feat: PKCE (Option 3 implementation) feat: PKCE (Option 3) Apr 19, 2023
@PrawiraGenestonlia PrawiraGenestonlia linked an issue Apr 25, 2023 that may be closed by this pull request
Copy link

@kschiew kschiew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM! Just a few suggestions on organising the code

src/index.ts Outdated Show resolved Hide resolved
src/util.ts Outdated Show resolved Hide resolved
src/constants.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@raynerljm raynerljm changed the title feat: PKCE (Option 3) feat: PKCE Apr 26, 2023
src/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@kwajiehao kwajiehao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
Copy link
Contributor

@PrawiraGenestonlia PrawiraGenestonlia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments

.eslintrc Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Copy link

@kschiew kschiew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New changes needed due to OpenID Discovery endpoint changes!

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
test/SgidClient.spec.ts Outdated Show resolved Hide resolved
test/SgidClient.spec.ts Outdated Show resolved Hide resolved
test/mocks/handlers.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@kwajiehao kwajiehao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me in general, left a few comments

Will leave it to @mantariksh to have the final say on PR approval

src/index.ts Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@PrawiraGenestonlia PrawiraGenestonlia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@PrawiraGenestonlia PrawiraGenestonlia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Will leave it to @mantariksh to have the final say on PR approval

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

starting with comments on the README first!

.eslintrc Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/generators.ts Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
src/constants.ts Outdated Show resolved Hide resolved
src/error.ts Outdated Show resolved Hide resolved
src/error.ts Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/sgidClient.ts Outdated Show resolved Hide resolved
test/SgidClient.spec.ts Outdated Show resolved Hide resolved
test/SgidClient.spec.ts Show resolved Hide resolved
test/SgidClient.spec.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gtml gooks tood mo le

release plan should minimally test against sgID production rather than a local sgID server, and ideally test in a production environment rather than a local environment

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gtml gooks tood mo le

release plan should minimally test against sgID production rather than a local sgID server, and ideally test in a production environment rather than a local environment

@raynerljm
Copy link
Contributor Author

gtml gooks tood mo le

release plan should minimally test against sgID production rather than a local sgID server, and ideally test in a production environment rather than a local environment

yup totally agree - here is a draft of the release plan which involves how the release will be done and testing in local + prod + a mix

@raynerljm raynerljm requested a review from kschiew May 15, 2023 01:58
Copy link

@kschiew kschiew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did one last sanity check , lgtm!

@raynerljm raynerljm merged commit 56545e4 into develop May 15, 2023
@raynerljm raynerljm deleted the feat/pkce-breaking branch May 15, 2023 02:55
@raynerljm raynerljm mentioned this pull request May 15, 2023
@raynerljm raynerljm restored the feat/pkce-breaking branch May 15, 2023 03:06
@raynerljm raynerljm mentioned this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement code verifier for PKCE
5 participants