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

Increasing hash key length #8038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

subhash-arabhi
Copy link
Contributor

@subhash-arabhi subhash-arabhi commented Dec 11, 2024

Issue

Hash key length is different in different sections of project. We are increasing two of them to 64

Changes

Increased hash key length to 64 in below projects
Bootstrap/org.netbeans.CLIHandler and
Gradle-Projects/org.netbeans.modules.gradle.ProjectTrust

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Please ensure the commits are grouped into atomic changes. There is no squash-on-merge in the NetBeans repository, as github side merges are unreliable regarding the author information.

What is missing in the commit message and the PR is the motivation. The commit message currently only gives the "what", but that is already answered by the diff, the "why" is the interesting part.

If I get it correctly, it was determined, that 14-18 bytes are not enough randomness. And the keys are used to verify local connections from NB cli and gradle.

Test changes

Changed test file
@subhash-arabhi subhash-arabhi force-pushed the increasing-hash-key-len branch from f060d25 to 30670fa Compare December 27, 2024 07:36
@subhash-arabhi
Copy link
Contributor Author

Please ensure the commits are grouped into atomic changes. There is no squash-on-merge in the NetBeans repository, as github side merges are unreliable regarding the author information.

What is missing in the commit message and the PR is the motivation. The commit message currently only gives the "what", but that is already answered by the diff, the "why" is the interesting part.

If I get it correctly, it was determined, that 14-18 bytes are not enough randomness. And the keys are used to verify local connections from NB cli and gradle.

@matthiasblaesing Yes, we want to increase the randomness. I added this in the description and squashed the commits. Please approve the PR

@matthiasblaesing
Copy link
Contributor

@subhash-arabhi please update the commit message. The commit message should explain why a change was done. What is more the summary line should give a meaningful summary. Reading the current summary I wonder which hash key lengths?

This still leaves the question: why is 64 right and 16 wrong. How did you determine that? Another question: is this just a random value at runtime or is it stored somewhere? If it is stored, why are the changed lengths not problematic?

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.

2 participants