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

Rename NewSoftwarePrivateKey to NewPrivateKey #53598

Open
wants to merge 1 commit into
base: joerger/hardware-key-service-reduced
Choose a base branch
from
Open

Rename NewSoftwarePrivateKey to NewPrivateKey #53598

wants to merge 1 commit into from

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Mar 31, 2025 •
edited
Loading

Previously NewSoftwarePrivateKey was added because YubiKeyPrivateKey was not uniform across build tags and was difficult to work with in marshal/parsing logic. With #53435, we added a more generic HardwarePrivateKey which can be marshaled and parsed as if they are software private keys.

TODO: Add a follow up to remove the (ignored) variadic argument on NewPrivateKey.

Depends on #53674

@Joerger Joerger changed the base branch from master to joerger/hardware-key-service March 31, 2025 18:23
@Joerger
Copy link
Contributor Author

Joerger commented Mar 31, 2025

@nklaassen Is my understanding in the PR description accurate? I didn't see have a separate NewSoftwarePrivateKey after my changes with the hardware key service.

@nklaassen
Copy link
Contributor

Yeah i just added NewSoftwarePrivateKey because NewPrivateKey required the caller to provide the key PEM at the time, which was a slight inconvenience requiring the caller to marshal the key to PEM. Happy to see it go away

nklaassen
nklaassen approved these changes Mar 31, 2025
View reviewed changes
api/utils/keys/privatekey.go
// NewPrivateKey returns a new PrivateKey for a crypto.Signer.
// [signer] must be an *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey, or *hardwarekey.PrivateKey.
// TODO(Joerger): Remove the variadic argument once /e is updated to not provide it.
func NewPrivateKey(signer crypto.Signer, _ ...[]byte) (*PrivateKey, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

note: it would be nice if we actually didn't have to marshal the key to PEM when a lot of the time it is not even necessary and just wastes CPU cycles when we never write out the key. The fact PrivateKeyPEM() currently doesn't return an error is what locks us in I guess we'd have to update every caller. Not expecting you to do anything about this here just calling it out

@Joerger Joerger force-pushed the joerger/hardware-key-service branch from d14db39 to cfcb9fb Compare April 2, 2025 01:03
@Joerger Joerger force-pushed the joerger/new-private-key branch from 00e4285 to 6d650c4 Compare April 2, 2025 01:05
@Joerger Joerger marked this pull request as ready for review April 2, 2025 01:06
@github-actions github-actions bot added database-access Database access related issues and PRs kubernetes-access machine-id size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Apr 2, 2025
@Joerger Joerger force-pushed the joerger/new-private-key branch from 6d650c4 to 64d5afc Compare April 2, 2025 21:09
@Joerger Joerger changed the base branch from joerger/hardware-key-service to joerger/hardware-key-service-reduced April 2, 2025 21:09
@Joerger Joerger force-pushed the joerger/hardware-key-service-reduced branch from 4729e97 to a986b49 Compare April 2, 2025 23:52
@Joerger Joerger force-pushed the joerger/new-private-key branch from 64d5afc to 6160260 Compare April 3, 2025 00:28
@Joerger Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Apr 3, 2025
@Joerger Joerger mentioned this pull request Apr 3, 2025
Draft
@Joerger Joerger force-pushed the joerger/new-private-key branch from 6160260 to aa3f8c4 Compare April 3, 2025 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greedy52 greedy52 greedy52 approved these changes

@nklaassen nklaassen nklaassen approved these changes

Assignees
No one assigned
Labels
database-access Database access related issues and PRs kubernetes-access machine-id no-changelog Indicates that a PR does not require a changelog entry size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Joerger @nklaassen @greedy52