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 EVP_PKEY_get_hmac() function #1217

Closed
wants to merge 1 commit into from
Closed

Conversation

npmccallum
Copy link
Contributor

Before the addition of this function, it was impossible to read the
symmetric key from an EVP_PKEY_HMAC type EVP_PKEY.

@ekasper
Copy link
Contributor

ekasper commented Jun 15, 2016

Why do you need to read this key?

@@ -237,6 +237,17 @@ void *EVP_PKEY_get0(const EVP_PKEY *pkey)
return pkey->pkey.ptr;
}

const unsigned char *EVP_PKEY_get_hmac(const EVP_PKEY *pkey, int *len)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be EVP_PKEY_get0_hmac or perhaps better yet EVP_PKEY_get0_hmac_key?

size_t for the length.

The type check should happen before you call EVP_PKEY_get0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this stand a chance of making it into 1.1? If not, we should be looking at other things ☺

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this falls in the "missing accessors now that stuff is opaque" category, so if it's really needed, yes it should make it to 1.1.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed in my latest commit.

Copy link
Member

Choose a reason for hiding this comment

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

It seems to be more consistent to use EVP_PKEY_get0_hmac(), without _key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't care which function name we use. Just let me know what the consensus is and I'll change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Go with EVP_PKEY_get0_hmac then, fine by me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest push uses EVP_PKEY_get0_hmac().

@ekasper ekasper added the api label Jun 15, 2016
@npmccallum
Copy link
Contributor Author

@ekasper https://github.com/npmccallum/jose

I'm writing a library for JOSE that wraps around OpenSSL. It provides functions to convert from JSON Web Keys (JWKs) to EVP_PKEYs and vice versa: https://github.com/npmccallum/jose/blob/master/src/jwk.h

I can convert from a JWK to a EVP_PKEY_HMAC, but not back since I have no direct access to the buffer: https://github.com/npmccallum/jose/blob/master/src/jwk.c#L156

I also need access to the length of the key in order to make a suggestion about what signing protocol to use: https://github.com/npmccallum/jose/blob/master/src/hmac.c#L10

@ekasper
Copy link
Contributor

ekasper commented Jun 15, 2016

Thanks for the clarification, that's reasonable. +1 from me.

Before the addition of this function, it was impossible to read the
symmetric key from an EVP_PKEY_HMAC type EVP_PKEY.
@npmccallum
Copy link
Contributor Author

Can this be merged now that I have addressed all of the stated concerns?

@richsalz
Copy link
Contributor

+1 i will get to it shortly.

levitte pushed a commit that referenced this pull request Jun 16, 2016
Before the addition of this function, it was impossible to read the
symmetric key from an EVP_PKEY_HMAC type EVP_PKEY.

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #1217)
@richsalz richsalz added the approval: done This pull request has the required number of approvals label Jun 16, 2016
@npmccallum
Copy link
Contributor Author

AFAICS, this is closed.

@npmccallum npmccallum closed this Jun 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants