-
Notifications
You must be signed in to change notification settings - Fork 131
Use SHA256 to compare SSH public key fingerprint #3249
Use SHA256 to compare SSH public key fingerprint #3249
Conversation
❌ Hey combor! All pull request submitters and commit authors must have a Contributor License Agreement (CLA). Click here for details on the CLA process. The following github user @combor is not covered by a CLA. After the CLA process is complete, this pull request will need to be closed & reopened. DreddBot will then validate the CLA(s). |
Codecov Report
@@ Coverage Diff @@
## v2-master #3249 +/- ##
=============================================
- Coverage 70.9% 70.89% -0.02%
=============================================
Files 642 642
Lines 28204 28204
Branches 6412 6412
=============================================
- Hits 19999 19995 -4
- Misses 8205 8209 +4 |
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.
The fingerprint check needs to cope with both MD5 and SHA256 - this change only supports SHA256, which will be break any deployments that use MD5.
SHS256 Fingerprints are prefixed with 'SHA256', so we should check which type the fingerprint is and use that when checking.
@nwmac SHA256 are not prefixed when returned from api /v2/info
It needs to add the prefix for comparison with But I agree in general with the point of keeping this backwards compatible. I'll prepare a change. |
e2b079b
to
11ba12d
Compare
@nwmac please check now. It checks both sums now serially and returns on first hit. |
@combor As I understand it, the fingerprint as returned by cf's /v2/info API will include the SHA256: prefix if that algorithm is being used, otherwise it is an MD5. So, I think the code should be:
|
@nwmac well, I don't thing /v2/info returns fingerprint with a prefix, that's why I'm using this to include it:
and later compare with
as this one includes Just to summarise and be clear:
|
@combor Sorry, you're right it doesn't include SHA256 - I'd found that while looking into this, but its not standard. I've done a bit more investigation - I think we should adopt the same approach as the CLI team did - using the key length to determine the algorithm - see here: https://github.com/cloudfoundry/cli/pull/1072/files |
Cool, the |
cf api can return SHA256 now depending on configuration so we add it to the picture and test for string length to use correct hashing function Signed-off-by: Piotr Komborski <piotr.komborski@fil.com>
11ba12d
to
b72dc68
Compare
Hi @nwmac, I just updated this PR. Please have a look and check if that's what you meant. |
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.
LGTM
Description
MD5 is deprecated and cf info endpoint returns now SHA256 fingerprint.
Use SHA256 to compare fingerprints instead MD5
Motivation and Context
This fixes ssh to an app instance functionality.
How Has This Been Tested?
Manually
Types of changes
Checklist: