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 support for Fortanix DSM signer #469

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

mzohreva
Copy link
Contributor

I'll need to update documentation to reflect the new signing provider, but wanted to get some early feedback first.

@tony-iqlusion
Copy link
Member

Mind rebasing? We just cut a new release and can start looking at this

@mzohreva mzohreva force-pushed the mz/support-dsm branch 2 times, most recently from cf06381 to f47e62e Compare February 16, 2022 17:22
@mzohreva
Copy link
Contributor Author

mzohreva commented Mar 3, 2022

@tony-iqlusion any progress on reviewing this? Let me know if you need more information about Fortanix DSM.

@tony-iqlusion
Copy link
Member

@mzohreva it looks like the checks didn't run. Can you try doing a quick git commit --amend and push --force-with-lease and see if that kicks them off?

@mzohreva
Copy link
Contributor Author

@tony-iqlusion just did what you asked, I see no change. In the Checks tab I see this: This workflow is awaiting approval from a maintainer in #469

@tony-iqlusion
Copy link
Member

@mzohreva I have a button on my side now, which I just hit, and the workflow is running

@mzohreva
Copy link
Contributor Author

Seems like ed25519 1.4.0 is yanked, I'll update

@tony-iqlusion
Copy link
Member

Looks like a clippy failure

@mzohreva
Copy link
Contributor Author

@tony-iqlusion just addressed the clippy issues.

@tony-iqlusion
Copy link
Member

Thanks! The build is finally green. Will take a more in-depth look tomorrow.

}

// See RFC 8410 section 3
const ED_25519_OID: ObjectIdentifier = ObjectIdentifier::new("1.3.101.112");
Copy link
Member

Choose a reason for hiding this comment

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

This is available as ed25519::pkcs8::ALGORITHM_OID.

Not a blocker for merging though, I can fix it up afterward (ed25519 is currently only a transitive dependency).

Comment on lines +214 to +233
struct Ed25519PublicKey(Ed25519);

impl DecodePublicKey for Ed25519PublicKey {}

impl<'a> TryFrom<SubjectPublicKeyInfo<'a>> for Ed25519PublicKey {
type Error = SpkiError;

fn try_from(spki: SubjectPublicKeyInfo<'_>) -> Result<Self, Self::Error> {
spki.algorithm.assert_algorithm_oid(ED_25519_OID)?;

if spki.algorithm.parameters != None {
// TODO: once/if https://github.com/RustCrypto/formats/issues/354 is addressed we should use that error variant.
return Err(SpkiError::KeyMalformed);
}

Ed25519::from_bytes(spki.subject_public_key)
.map_err(|_| SpkiError::KeyMalformed)
.map(Ed25519PublicKey)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Likewise most of this is available as ed25519::pkcs8::PublicKeyBytes

@tony-iqlusion
Copy link
Member

A few nits re: PKCS#8/SPKI, but I'm going to go ahead and merge anyway as I'd like to get this in both to have it and to unblock other work.

If you'd like to follow up with using the ed25519 crate that'd be great, otherwise I can take a look.

@tony-iqlusion tony-iqlusion merged commit a036b5d into iqlusioninc:main Apr 28, 2022
@tony-iqlusion tony-iqlusion mentioned this pull request May 24, 2022
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.

2 participants