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

Add asymmetric JWT signing #16010

Merged
merged 16 commits into from
Jun 17, 2021
Merged

Add asymmetric JWT signing #16010

merged 16 commits into from
Jun 17, 2021

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented May 28, 2021

Close #15912

Added asymmetric JWT signing. Supports HS256 (already implemented), HS384, HS512, RS256, RS384, RS512, ES256, ES384 and ES512.

The default signing algorithm is changed from HS256 (symmetric encryption) to RS256 (asymmetric encryption).

⚠️ BREAKING ⚠️

  • Asymmetric algorithms require a secret asymmetric key pair to be in JWT_SIGNING_PRIVATE_KEY_FILE (by default APP_DATA_PATH/jwt), a pair will be generated if it is not present. (NB: this was originally in CUSTOM_PATH but was changed by Fix mkdir jwt - permission denied #16227)
  • The original symmetric JWT_SECRET will only be used if JWT_SIGNING_ALGORITHM is set to HS256 (previous default), HS384 or HS512.
  • As a result of the change of algorithm gitea OAuth2 tokens (and potentially client secrets) will need to be regenerated unless you change your JWT_SIGNING_ALGORITHM back to HS256.
  • Legacy installations of Drone assume a short key length, however as no automated migrations are run by Drone you'll need to do them manually if the column type for user_oauth_token and user_oauth_refresh are limited to 500 characters:
-- an example manual migration for Drone database connected to postgresql
alter table users alter column user_oauth_token type bytea using convert_to(user_oauth_token, 'LATIN1');
alter table users alter column user_oauth_refresh type bytea using convert_to(user_oauth_refresh, 'LATIN1');

or see: #16010 (comment)

@KN4CK3R KN4CK3R marked this pull request as draft May 28, 2021 20:14
@techknowlogick techknowlogick added this to the 1.15.0 milestone May 28, 2021
@techknowlogick techknowlogick added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels May 28, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 30, 2021
@KN4CK3R KN4CK3R changed the title WIP: Add asymmetric JWT signing Add asymmetric JWT signing May 30, 2021
@KN4CK3R KN4CK3R marked this pull request as ready for review May 30, 2021 19:26
@KN4CK3R KN4CK3R removed the pr/wip This PR is not ready for review label May 30, 2021
@codecov-commenter
Copy link

codecov-commenter commented May 30, 2021

Codecov Report

Merging #16010 (3cfaecc) into main (1ec9e90) will decrease coverage by 0.06%.
The diff coverage is 32.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16010      +/-   ##
==========================================
- Coverage   44.57%   44.50%   -0.07%     
==========================================
  Files         700      701       +1     
  Lines       82867    83092     +225     
==========================================
+ Hits        36940    36984      +44     
- Misses      39936    40105     +169     
- Partials     5991     6003      +12     
Impacted Files Coverage Δ
cmd/generate.go 0.00% <0.00%> (ø)
models/oauth2.go 28.00% <0.00%> (-1.17%) ⬇️
modules/generate/generate.go 0.00% <0.00%> (-16.00%) ⬇️
modules/setting/lfs.go 52.94% <0.00%> (ø)
routers/install/install.go 0.00% <0.00%> (ø)
routers/web/user/oauth.go 36.53% <26.47%> (-2.43%) ⬇️
modules/auth/oauth2/jwtsigningkey.go 32.50% <32.50%> (ø)
models/oauth2_application.go 69.39% <88.88%> (+0.21%) ⬆️
modules/setting/setting.go 50.28% <100.00%> (-1.23%) ⬇️
routers/web/web.go 91.65% <100.00%> (+0.01%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdf9ab1...3cfaecc. Read the comment docs.

@@ -350,7 +350,7 @@ relation to port exhaustion.
- `ISSUE_INDEXER_PATH`: **indexers/issues.bleve**: Index file used for issue search; available when ISSUE_INDEXER_TYPE is bleve and elasticsearch.
- The next 4 configuration values are deprecated and should be set in `queue.issue_indexer` however are kept for backwards compatibility:
- `ISSUE_INDEXER_QUEUE_TYPE`: **levelqueue**: Issue indexer queue, currently supports:`channel`, `levelqueue`, `redis`.
- `ISSUE_INDEXER_QUEUE_DIR`: **queues/common**: When `ISSUE_INDEXER_QUEUE_TYPE` is `levelqueue`, this will be the path where the queue will be saved. (Previously this was `indexers/issues.queue`.)
Copy link
Member

Choose a reason for hiding this comment

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

per discussion in chat, these changed lines are fine due to the fact that it cleans up line endings (shakes fist at windows)

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Thank you so much so far on your work on this PR!!

I was able to sign into Sourcegraph using this PR, however testing this PR more I found that 1. sourcegraph may not be using the cert to validate the signature on this token, 2. possibly the upstream jwt library may not be signing the token correctly.

models/oauth2_application.go Show resolved Hide resolved
func loadOrCreateSymmetricKey() (interface{}, error) {
key := make([]byte, 32)
n, err := base64.RawURLEncoding.Decode(key, []byte(setting.OAuth2.JWTSecretBase64))
if err != nil || n != 32 {
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently only 32 bytes are allowed. Should we really enforce this? Smaller keys are valid and larger keys are hashed by the internal hashing algorithm to create a secret with the correct length.
https://github.com/golang/go/blob/2ebe77a2fda1ee9ff6fd9a3e08933ad1ebaea039/src/crypto/hmac/hmac.go#L148-L152

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 13, 2021
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 17, 2021
@6543 6543 merged commit 29695cd into go-gitea:main Jun 17, 2021
@KN4CK3R KN4CK3R deleted the feature-jwt-asymmetric branch June 18, 2021 06:28
@zeripath zeripath added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Jun 21, 2021
@proton-ab
Copy link
Contributor

proton-ab commented Jul 13, 2021

Friendly warning to anyone out there using Drone - it fails to support new tokens which are too long
"msg": "cannot update user: pq: value too long for type character varying(500)",

@markeissler
Copy link

TL;DR: For Drone compatibility... Set the gitea environment variable to:

GITEA__OAUTH2__JWT_SIGNING_ALGORITHM=HS256

And restart gitea - OR - alter your database to accommodate the longer value that needs to be stored. Something like the following might work, or perhaps set the column type to TEXT instead of BYTEA.

ALTER TABLE users ALTER COLUMN user_oauth_token BYTEA;

I would suggest modifying the column in the drone db for production environments since the intention is to improve security with asymmetric tokens. By default, the column is setup as a varchar(500) type.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Jul 13, 2021

A shorter RSA key may help too. The default key size is 4096 bit.
The drone problem was fixed in this commit two months ago:
harness/harness@eaadd82

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 13, 2021
@6543
Copy link
Member

6543 commented Jul 13, 2021

thanks for the hint, for more issues etc please open a new issues

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support RS256/JWKS for signing/verifying OAUTH JWTs
10 participants