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 sha384 to sig auth #26

Closed
wants to merge 5 commits into from
Closed

Add sha384 to sig auth #26

wants to merge 5 commits into from

Conversation

cdr-chakotay
Copy link
Collaborator

Needed changes for signature authentication makeover.

Have had no rights in the repo, providing the patch like this now :)

@arnaudlimbourg @Acconut

Year 2024 ;)
@arnaudlimbourg
Copy link
Collaborator

@cdr-chakotay thanks!

Should there be support for more hashing algorithms? I'm wondering about the potential proliferation of boolean flags :)

@cdr-chakotay
Copy link
Collaborator Author

@cdr-chakotay thanks!

Should there be support for more hashing algorithms? I'm wondering about the potential proliferation of boolean flags :)

We are currently moving away from SHA1 to SHA384. To avoid issues during the transit I wanted the users to be able to fall back to SHA1 in case of emergency. This is why I added the flag :)

@Acconut
Copy link
Member

Acconut commented Mar 1, 2024

I agree with @arnaudlimbourg. If we add an option to control the signature algorithm, it is better to have a non-boolean value for forward compatibility. Something like signature_algorithm = 'sha1' | 'sha384'

@arnaudlimbourg
Copy link
Collaborator

an Enum could work here, not sure whether it would add much value.

@cdr-chakotay
Copy link
Collaborator Author

We can also just swap to SHA384, as the current discussion lines out that it should not have a negative effect.

@Acconut
Copy link
Member

Acconut commented Mar 4, 2024

We can also just swap to SHA384, as the current discussion lines out that it should not have a negative effect.

Yes, we have chosen that approach for most other SDKs now.

@cdr-chakotay
Copy link
Collaborator Author

cdr-chakotay commented Mar 7, 2024

Yeah, okay will update the PR is on my to-do :)

@arnaudlimbourg can you please add me to the SDK, so I do not need to fork it in my private repo in the future ?

Reflect decision to solely support SHA384
Update signature auth section to support sha384 only
@cdr-chakotay
Copy link
Collaborator Author

Hey any updates regarding this :)

@Acconut
Copy link
Member

Acconut commented Mar 19, 2024

I just added you as a maintainer to this repo. Did it work?

@cdr-chakotay
Copy link
Collaborator Author

Yeah, worked out :) Do we want to merge this ?

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

The change itself looks good, but I am a bit surprised that no changes to tests are needed to make CI pass. Do we not have any tests covering signature generation?

@cdr-chakotay
Copy link
Collaborator Author

cdr-chakotay commented Mar 22, 2024

The change itself looks good, but I am a bit surprised that no changes to tests are needed to make CI pass. Do we not have any tests covering signature generation?

Well I can certainly set up some or @arnaudlimbourg wants to do this :D?

@Acconut
Copy link
Member

Acconut commented Mar 22, 2024

Feel free to give it a try!

@cdr-chakotay
Copy link
Collaborator Author

Migrated to internal PR 27

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.

3 participants