Expose crypto constants through function calls #23

Closed
namelessjon opened this Issue Mar 15, 2013 · 24 comments

Comments

Projects
None yet
5 participants
@namelessjon

Currently, many of the constants used throughout this code are only available as #define. This is okay for C code, but means that constants have to be manually transcribed for an FFI interface. It would be helpful if there were some function(s) that could be called to retrieve this information.

This came up in cryptosphere/rbnacl#49

@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 Mar 15, 2013

Owner

It definitely makes sense to expose these through functions, in addition to #define. Will do it tomorrow.

Owner

jedisct1 commented Mar 15, 2013

It definitely makes sense to expose these through functions, in addition to #define. Will do it tomorrow.

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset Mar 22, 2013

Contributor

Hooray! 👍

Contributor

stouset commented Mar 22, 2013

Hooray! 👍

@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 Apr 22, 2013

Owner

This has been done, at least for each operation. Check out the current code and tell me if you need more values exposed!

Owner

jedisct1 commented Apr 22, 2013

This has been done, at least for each operation. Check out the current code and tell me if you need more values exposed!

@tarcieri

This comment has been minimized.

Show comment Hide comment
@tarcieri

tarcieri Apr 22, 2013

Contributor

@jedisct1 will look soon. We just shipped 1.1 with hardcoded constants in Ruby, but for 2.0 we can definitely leverage these

Contributor

tarcieri commented Apr 22, 2013

@jedisct1 will look soon. We just shipped 1.1 with hardcoded constants in Ruby, but for 2.0 we can definitely leverage these

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset Apr 22, 2013

Contributor

On Apr 21, 2013, at 5:37 PM, Frank Denis notifications@github.com wrote:

This has been done, at least for each operation. Check out the current code and tell me if you need more values exposed!

Fantastic.

I'll let you know how it goes using these in my own project.

Stephen Touset
stephen@touset.org

Contributor

stouset commented Apr 22, 2013

On Apr 21, 2013, at 5:37 PM, Frank Denis notifications@github.com wrote:

This has been done, at least for each operation. Check out the current code and tell me if you need more values exposed!

Fantastic.

I'll let you know how it goes using these in my own project.

Stephen Touset
stephen@touset.org

@jedisct1 jedisct1 closed this Apr 24, 2013

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 15, 2013

Contributor

Actually, I found have a problem with the way the current approach is implemented. You appear to provide methods exposing the constant values for each cryptographic primitive (e.g., crypto_auth_bytes, crypto_auth_keybytes), but only for the default implementation.

Part of the original purpose behind this change is to allow wrappers on libsodium to provide backward-compatibility in case one of the default implementations changes. So it would be extremely useful to also have

crypto_auth_hmacsha512_256_bytes
crypto_auth_hmacsha512_256_keybytes
crypto_auth_hmacsha256_bytes
crypto_auth_hmacsha256_keybytes
...

Obviously not just for the crypto_auth_* family of functions. I'm just using them as an example.

Contributor

stouset commented May 15, 2013

Actually, I found have a problem with the way the current approach is implemented. You appear to provide methods exposing the constant values for each cryptographic primitive (e.g., crypto_auth_bytes, crypto_auth_keybytes), but only for the default implementation.

Part of the original purpose behind this change is to allow wrappers on libsodium to provide backward-compatibility in case one of the default implementations changes. So it would be extremely useful to also have

crypto_auth_hmacsha512_256_bytes
crypto_auth_hmacsha512_256_keybytes
crypto_auth_hmacsha256_bytes
crypto_auth_hmacsha256_keybytes
...

Obviously not just for the crypto_auth_* family of functions. I'm just using them as an example.

@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 May 15, 2013

Owner

Functions returning constant values have indeed only been implemented for the default constructions.
That's only temporary and only because adding them takes time and I wanted to start with these.
If you want to help with writing the remaining wrappers, your help would be more than welcome!

Owner

jedisct1 commented May 15, 2013

Functions returning constant values have indeed only been implemented for the default constructions.
That's only temporary and only because adding them takes time and I wanted to start with these.
If you want to help with writing the remaining wrappers, your help would be more than welcome!

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 15, 2013

Contributor

I'm actually already on it. I'm having trouble getting the project to build from a clean git checkout though. Is there a quick guide to getting it to build?

$ autoconf
configure.ac:9: error: possibly undefined macro: AM_INIT_AUTOMAKE
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
configure.ac:11: error: possibly undefined macro: AM_MAINTAINER_MODE
configure.ac:12: error: possibly undefined macro: AM_DEP_TRACK
configure.ac:40: error: possibly undefined macro: AM_PROG_AS
configure.ac:231: error: possibly undefined macro: AM_CONDITIONAL
configure.ac:347: error: possibly undefined macro: AC_LIBTOOL_WIN32_DLL
$ ./configure 
configure: error: cannot find install-sh, install.sh, or shtool in "." "./.." "./../.."
Contributor

stouset commented May 15, 2013

I'm actually already on it. I'm having trouble getting the project to build from a clean git checkout though. Is there a quick guide to getting it to build?

$ autoconf
configure.ac:9: error: possibly undefined macro: AM_INIT_AUTOMAKE
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
configure.ac:11: error: possibly undefined macro: AM_MAINTAINER_MODE
configure.ac:12: error: possibly undefined macro: AM_DEP_TRACK
configure.ac:40: error: possibly undefined macro: AM_PROG_AS
configure.ac:231: error: possibly undefined macro: AM_CONDITIONAL
configure.ac:347: error: possibly undefined macro: AC_LIBTOOL_WIN32_DLL
$ ./configure 
configure: error: cannot find install-sh, install.sh, or shtool in "." "./.." "./../.."
@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 May 15, 2013

Owner

Awesome!

Run ./autogen.sh, it should build everything you need.

Owner

jedisct1 commented May 15, 2013

Awesome!

Run ./autogen.sh, it should build everything you need.

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 15, 2013

Contributor

👍

Contributor

stouset commented May 15, 2013

👍

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 15, 2013

Contributor

One more quick question. I'm hesitant to edit any of the files under crypto_$primitive/$implementation/ref, since those are checksummed and presumedly pristine imports from DJB's NaCl project. But that seems like the most logical place to put them. Where should these definitions go?

Contributor

stouset commented May 15, 2013

One more quick question. I'm hesitant to edit any of the files under crypto_$primitive/$implementation/ref, since those are checksummed and presumedly pristine imports from DJB's NaCl project. But that seems like the most logical place to put them. Where should these definitions go?

@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 May 15, 2013

Owner

ref refers to a specific implementation. I don't think we should expose anything related to a specific implementation. ed25519 is ed25519 and code shouldn't link against a specific implementation. The library will take care of picking a fast one that works.

For constants, it doesn't make any sense to have different functions for each implementation of a primitive. These will hopefully always be the same.

So the best place to add them is probably crypto_$operation/$primitive/$operation_$primitive.c as in crypto_onetimeauth/poly1305/onetimeauth_poly1305.c

Owner

jedisct1 commented May 15, 2013

ref refers to a specific implementation. I don't think we should expose anything related to a specific implementation. ed25519 is ed25519 and code shouldn't link against a specific implementation. The library will take care of picking a fast one that works.

For constants, it doesn't make any sense to have different functions for each implementation of a primitive. These will hopefully always be the same.

So the best place to add them is probably crypto_$operation/$primitive/$operation_$primitive.c as in crypto_onetimeauth/poly1305/onetimeauth_poly1305.c

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 15, 2013

Contributor

Before you replied, I approached it this way. I can do it your way if you prefer, too. I agree that the constants shouldn't differ between implementations. Let me know your thoughts.

Contributor

stouset commented May 15, 2013

Before you replied, I approached it this way. I can do it your way if you prefer, too. I agree that the constants shouldn't differ between implementations. Let me know your thoughts.

@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 May 15, 2013

Owner

The problem with static inline is that the symbols are exported, making the library difficult to use from other programming languages.

Owner

jedisct1 commented May 15, 2013

The problem with static inline is that the symbols are exported, making the library difficult to use from other programming languages.

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 15, 2013

Contributor

A'ight. :)

Contributor

stouset commented May 15, 2013

A'ight. :)

@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 May 15, 2013

Owner

Errr... That the symbols are not exported, sorry. But you got the point :)

Owner

jedisct1 commented May 15, 2013

Errr... That the symbols are not exported, sorry. But you got the point :)

@tarcieri

This comment has been minimized.

Show comment Hide comment
@tarcieri

tarcieri May 15, 2013

Contributor

Note specifically that static is unusable from FFI

Contributor

tarcieri commented May 15, 2013

Note specifically that static is unusable from FFI

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 15, 2013

Contributor

D'oh! Take a look at this approach, instead.

Contributor

stouset commented May 15, 2013

D'oh! Take a look at this approach, instead.

@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 May 15, 2013

Owner

That's really cool, bro!

Owner

jedisct1 commented May 15, 2013

That's really cool, bro!

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 15, 2013

Contributor

(edited: updated the commit to actually build)

Contributor

stouset commented May 15, 2013

(edited: updated the commit to actually build)

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 16, 2013

Contributor

I'll try and automate something, instead of having to type that all out by hand, manually.

Contributor

stouset commented May 16, 2013

I'll try and automate something, instead of having to type that all out by hand, manually.

@sneves

This comment has been minimized.

Show comment Hide comment
@sneves

sneves May 16, 2013

Contributor

Nitpick: if you're only bringing in stdlib.h for the size_t definition, you may want to include stddef.h instead. Both options are valid, of course, but stddef.h should be smaller and more specific.

Contributor

sneves commented May 16, 2013

Nitpick: if you're only bringing in stdlib.h for the size_t definition, you may want to include stddef.h instead. Both options are valid, of course, but stddef.h should be smaller and more specific.

@jedisct1

This comment has been minimized.

Show comment Hide comment
@jedisct1

jedisct1 May 16, 2013

Owner

Good nitpick! The changes have been done.

Owner

jedisct1 commented May 16, 2013

Good nitpick! The changes have been done.

@stouset

This comment has been minimized.

Show comment Hide comment
@stouset

stouset May 16, 2013

Contributor

I've recommitted the change but with stddef.h instead of stdlib.h. Barring any other suggestions, I'm going to try and semi-automate what I've got to hopefully add as many of these as possible without having to do it manually.

Contributor

stouset commented May 16, 2013

I've recommitted the change but with stddef.h instead of stdlib.h. Barring any other suggestions, I'm going to try and semi-automate what I've got to hopefully add as many of these as possible without having to do it manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment