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

(FACT-3110) Support for ecdsa 384 and 521 bit keys #2656

Merged
merged 4 commits into from
Dec 13, 2023
Merged

(FACT-3110) Support for ecdsa 384 and 521 bit keys #2656

merged 4 commits into from
Dec 13, 2023

Conversation

joshcooper
Copy link
Contributor

  • Adds support for ecdsa 384 and 521 bit keys
  • Remove overstubbing in the ssh resolvers
  • Convert Ssh and FingerPrint to value objects using Struct

Supersedes #2652

h0tw1r3 and others added 2 commits December 4, 2023 21:51
@h0tw1r3
(FACT-3110) support for ecdsa 384 and 521 bit keys
1bf69a4
@joshcooper
(FACT-3110) Create value objects
192e10c 
We still need to support older ruby versions so I can't use immutable Data
objects. As a result of this change, different objects containing the same
values are considered to be equivalent.
@joshcooper joshcooper requested a review from a team as a code owner December 13, 2023 01:24
@joshcooper
(FACT-3110) Add tests for base64 decode ignoring invalid characters
d6c3e80
@joshcooper
(FACT-3110) Refactor ssh tests
6e3223a 
The tests stubbed out the SshHelper.create_ssh method, which ignores invalid
base64 characters like newlines. To account for the overstubbing, the tests
called `strip!` on the fixture so the resulting file content matched
what create_ssh would do.

The tests were also using the wrong fingerprint values for some keys, because
the create_ssh only calculates the fingerprint over the <key> portion of
the file, excluding the <type>. However, the tests still passed because they
were expecting the incorrect value.

I followed this process to generate the correct fingerprints:

    # ssh-keygen -l -E sha256 -f ecdsa384
    384 SHA256:lJ2S1lxrs5CHJ771za/vW1RmUNZKCBpPhefcr2t8t6s no comment (ECDSA)
    # echo -n lJ2S1lxrs5CHJ771za/vW1RmUNZKCBpPhefcr2t8t6s= | base64 -d | xxd -p -c 100
    949d92d65c6bb3908727bef5cdafef5b546650d64a081a4f85e7dcaf6b7cb7ab

Note sometimes ssh-keygen outputs base64 values without proper `=` padding. So
in the example above, I had to append `=`.
@joshcooper joshcooper force-pushed the FACT-3110-more-ecdsa-types branch from 2ec902c to 6e3223a Compare December 13, 2023 01:33
@joshcooper joshcooper added the enhancement New feature or enhancement label Dec 13, 2023
@cthorn42
Copy link
Collaborator

@joshcooper do you know if we'd get any coverage or would see any benefit of build puppet-agent with this and putting that through adhoc? Would that at least validate previous keys or whatever we default still work?

@cthorn42
Copy link
Collaborator

Talked with Josh about testing this offline, he did this before hand based upon Jeff's work. He was able to validate that Facter functioned with the new RSA and ECDSA keys.

@cthorn42 cthorn42 merged commit 0cd73de into puppetlabs:main Dec 13, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cthorn42 cthorn42 cthorn42 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshcooper @cthorn42 @h0tw1r3