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.

Sign up for GitHub

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

Jump to bottom

fix: ensure RSA key length fullfills 4096bit requirement (#2905) #3402

Merged
merged 5 commits into from
Mar 16, 2023
Merged

fix: ensure RSA key length fullfills 4096bit requirement (#2905) #3402

merged 5 commits into from
Mar 16, 2023

Conversation

aarmam
Copy link
Contributor

@aarmam aarmam commented Dec 30, 2022

Fixes #2905

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

@aarmam aarmam requested a review from aeneasr as a code owner December 30, 2022 12:22
@aarmam aarmam force-pushed the fix/hsm-min-rsa-key-length branch from afc70c6 to 3bf96db Compare December 30, 2022 12:30
alnr
alnr requested changes Jan 25, 2023
View reviewed changes
Copy link
Contributor

@alnr alnr left a comment

Choose a reason for hiding this comment

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

Cool! Sorry, not familiar with the spec. Is the min key size defined there as 4096 bits?

hsm/manager_hsm_test.go Show resolved Hide resolved
hsm/manager_hsm.go Outdated
Context: hsm,
KeySetPrefix: config.HSMKeySetPrefix(),
Context: hsm,
ctx: ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest not storing the context in the KeyManager. I'd suggest passing the configuration options (KeySetPrefix and DevMode for example) to the constructor instead and storing them directly.

Copy link
Contributor Author

@aarmam aarmam Jan 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I might remember it wrong, but in one of my previous code review, the suggestion was to pass configuration context so when configuration is updated, the changes will be used. Also because I am not full time go developer I code by example quite often. That said I am happy to make the change - just clearifing how I got here! :)

Copy link
Member

Choose a reason for hiding this comment

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

Hey, sorry for the late review. @alnr is correct here, the context should come from the function caller, not the service itself. I checked quickly and getKeySetAttributes is being called from two functions, both of which have context.Context available. I'll push the changes shortly

@codecov
Copy link

codecov bot commented Jan 26, 2023 •
edited
Loading

Codecov Report

Merging #3402 (97ac03a) into master (751c8e8) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 97ac03a differs from pull request most recent head c4c41c6. Consider uploading reports for the commit c4c41c6 to get more accurate results

@@           Coverage Diff           @@
##           master    #3402   +/-   ##
=======================================
  Coverage   76.85%   76.85%           
=======================================
  Files         123      123           
  Lines        9125     9105   -20     
=======================================
- Hits         7013     6998   -15     
+ Misses       1665     1663    -2     
+ Partials      447      444    -3

see 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aarmam
Copy link
Contributor Author

aarmam commented Jan 26, 2023

Cool! Sorry, not familiar with the spec. Is the min key size defined there as 4096 bits?

This constraint came from here

@alnr
Copy link
Contributor

alnr commented Feb 7, 2023

@aarmam Would you mind rebasing your PR onto master? We've improved our tooling so the copyright header won't require updating every year. Thanks so much!

Pinging @aeneasr for review, too.

@aeneasr aeneasr force-pushed the fix/hsm-min-rsa-key-length branch from e5a6fd5 to 81cca52 Compare March 16, 2023 10:59
aeneasr
aeneasr previously approved these changes Mar 16, 2023
View reviewed changes
@aeneasr aeneasr merged commit a663927 into ory:master Mar 16, 2023
harnash pushed a commit to Wikia/ory-hydra that referenced this pull request Apr 12, 2023
@aarmam @alnr
@aeneasr @harnash
fix: ensure RSA key length fullfills 4096bit requirement (ory#2905) (o…
e10361b 
…ry#3402)

Closes ory#2905

Co-authored-by: Arne <a.luenser@gmail.com>
Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alnr alnr alnr requested changes

@aeneasr aeneasr aeneasr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure PKCS RSA key length fullfills 4096bit requirement
3 participants
@aarmam @alnr @aeneasr