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

Expanded minimum RSA Keylength to 3072 #26604

Merged
merged 15 commits into from Aug 28, 2023
Merged

Conversation

mainboarder
Copy link
Contributor

@mainboarder mainboarder commented Aug 20, 2023

German Federal Office for Information Security requests in its technical guideline BSI TR-02102-1 RSA Keylength not shorter than 3000bits starting 2024, in the year 2023 3000bits as a recommendation. Gitea should request longer RSA Keys by default in favor of security and drop old clients which do not support longer keys.

https://www.bsi.bund.de/SharedDocs/Downloads/DE/BSI/Publikationen/TechnischeRichtlinien/TR02102/BSI-TR-02102.pdf?__blob=publicationFile&v=9 - Page 19, Table 1.2

German Federal Office for Information Security requests in its technical guideline BSI TR-02102-1 RSA Keylength not shorter than 3000bits starting 2024, in the year 2023 3000bits as a recommendation. Gitea should request longer RSA Keys by default in favor of security and drop old clients which do not support shorter keys.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 20, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 20, 2023
@silverwind silverwind added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Aug 20, 2023
@techknowlogick techknowlogick added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Aug 20, 2023
@wxiaoguang
Copy link
Contributor

CI fails, the test code also needs to be updated accordingly.

Would you like to fix them or would you like some maintainers to edit this PR to help?

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 21, 2023
@mainboarder
Copy link
Contributor Author

Searched again and found way more occurences of 2048 bit RSA.
I do need help with:

{"rsa-3072", false, "rsa", 3072, "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDMZXh+1OBUwSH9D45wTaxErQIN9IoC9xl7MKJkqvTvv6O5RR9YW/IK9FbfjXgXsppYGhsCZo1hFOOsXHMnfOORqu/xMDx4yPuyvKpw4LePEcg4TDipaDFuxbWOqc/BUZRZcXu41QAWfDLrInwsltWZHSeG7hjhpacl4FrVv9V1pS6Oc5Q1NxxEzTzuNLS/8diZrTm/YAQQ/+B+mzWI3zEtF4miZjjAljWd1LTBPvU23d29DcBmmFahcZ441XZsTeAwGxG/Q6j8NgNXj9WxMeWwxXV2jeAX/EBSpZrCVlCQ1yJswT6xCp8TuBnTiGWYMBNTbOZvPC4e0WI2/yZW/s5F nocomment"},
and potential other RSA Keys in that file, as I do not know, how the corresponding private key is handeld.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Vote the approval ahead since it looks good.

While I do not think it is "breaking" because the existing short ssh keys could still be used if I understand correctly.

@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 Aug 21, 2023
@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 Aug 21, 2023
@lunny lunny added this to the 1.21.0 milestone Aug 24, 2023
@wxiaoguang
Copy link
Contributor

CI still fails

@puni9869
Copy link
Member

Unit test need a refactor.

@puni9869 puni9869 added the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Aug 25, 2023
@mainboarder
Copy link
Contributor Author

Honestly dont know what to do. Please help.

@puni9869
Copy link
Member

puni9869 commented Aug 25, 2023

No worries,

Honestly dont know what to do. Please help.

Please run make test in the console.

This is the trace from error
https://github.com/go-gitea/gitea/actions/runs/5927063262/job/16070289374?pr=26604#step:8:245

@GiteaBot GiteaBot removed the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Aug 25, 2023
modules/util/keypair_test.go Outdated Show resolved Hide resolved
modules/util/keypair_test.go Outdated Show resolved Hide resolved
models/asymkey/ssh_key_test.go Outdated Show resolved Hide resolved
@wxiaoguang wxiaoguang added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. and removed pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! labels Aug 25, 2023
@puni9869
Copy link
Member

🚀

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 25, 2023

CI passes. And IMO this is not "breaking", all uploaded/generated keys are not affected, so removed the "breaking" label.

Feel free to add the breaking label back if it is really breaking (with detailed breaking description and what users should do to handle the breaking)

@mainboarder
Copy link
Contributor Author

Thank both of you!

@lunny lunny enabled auto-merge (squash) August 28, 2023 00:38
@lunny lunny merged commit c533991 into go-gitea:main Aug 28, 2023
24 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 28, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 28, 2023
* giteaofficial/main:
  Fix bug for ctx usage (go-gitea#26762)
  Remove some transition related code (go-gitea#26755)
  Expanded minimum RSA Keylength to 3072 (go-gitea#26604)
  [skip ci] Updated licenses and gitignores
  Use docs.gitea.com instead of docs.gitea.io (go-gitea#26739)
  Adding hint `Archived` to archive label. (go-gitea#26741)

# Conflicts:
#	templates/base/head_navbar.tmpl
@mainboarder mainboarder deleted the mb-rsa-keylength branch August 29, 2023 19:00
@silverwind silverwind added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Nov 20, 2023
@silverwind
Copy link
Member

silverwind commented Nov 20, 2023

I added a missing pr/breaking label to this. There was at least one user in Discord affected by this.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 26, 2023
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! size/S Denotes a PR that changes 10-29 lines, ignoring generated files. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants