-
Notifications
You must be signed in to change notification settings - Fork 6
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
Match changes requested in the RFC process for 1.3 signing #23
Conversation
This function will tell if an Algorithm, Version pair is allowed
Hashing these things is no longer required. The reason they were hashing in v1.0/v1.1 is because the message was directly signed. This meant that there was a limit on the size of the message. v1.3 does not have this requirement as RSASSA-PKCS#1-v1_5 signs a hash of the message.
Generally looks good. The cleanup to eliminate the subcomponent hashing is quite worthwhile. The one change I might make is adding RSA to signing algorithm name (RSA_SHA256or the like) |
I'm not sure I understand. Do you mean instead of |
|
oh i see the confusion. That constant should really just be called |
cc @chef/lob |
@jaym Sorry for not following up on that. Line 31 in 1139c81
to -define(SIGNING_ALGORITHM_RSA_SHA256, <<"rsa_sha256">>). (along with the natural cascading fixups) The idea is if in the future we needed to add ecdsa_sha256 or whatever it would be more obvious which encryption algorithm was in play. |
@manderson26 renamed... I was worried the change wouldn't make sense, but I think it still does. Perhaps a quick glance through what changed, make sure it still reads in a sane way |
%% This structure defines allowable hashing algorithms for each signing version. | ||
%% The frist one in the list is considered to be the default if none is | ||
%% specified | ||
-define(SIGNING_VERSIONS, [{?SIGNING_VERSION_V1_0, [?SIGNING_ALGORITHM_SHA1]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of proplist, can we just have call into a matched fun header in chef_authn.erl
? e.g.
signing_protocols_for_version(?SIGNING_VERSION_V1_0) ->
[?SIGNING_ALGORITHM_SHA1];
signing_protocols_for_version(?SIGNING_VERSION_V1_1) ->
[?SIGNING_ALGORITHM_SHA1];
% etc
This looks good, I just had the one minor comment above. 👍 once implemented |
Match changes requested in the RFC process for 1.3 signing
The message format has been updated. We no longer hash user id and path. Hashing these things is no longer required. The reason they were hashing in v1.0/v1.1 is because the message was directly signed. This meant that there was a limit on the size of the message. v1.3 does not have this requirement as RSASSA-PKCS#1-v1_5 signs a hash of the message.