Experiences using libyubikey
Tollef Fog Heen
Hi,
I'm in the middle of writing a yubikey validation server in C, because I
for various reasons don't want one in Java or PHP. I thought I'd write
some bits about my experiences using libyubikey. The list is not too
structured at the moment, I'm sorry about that, but figured it would be
better to send it than to forget. :-)
- It's confusing that yubikey_token_t is not a real type, but just a
pointer to a type (yubikey_token_st)
- yubikey_parse takes in one a modhex encoded token and an unencoded AES
key. This is somewhat confusing too. It also requires you to strip
the public uid from the front of the token rather than just doing it
for you.
- yubikey_crc_ok_p(tok) does not actually take in a token, it takes in a
uint8_t, requiring you to do odd stuff like:
yubikey_crc_ok_p((uint8_t*)&token). (token is a yubikey_token_st
here.) IMO any API which forces you to use casting when doing normal,
routine operations needs fixing.
- the yubikey_token_t struct is a bit on the low-level side -- why is
the timestamp split across two fields, and why is the «key triggered
by caps lock» bit of the session counter not moved into its own field?
- the use field of the token structure seems a bit underspecified, the
comment says: «Number of times used within session + activation
flags.», but I'm not able to find typedefs for any activation flags
(or similar) in the header file.
- functions don't say whether they allocate memory or not, which is
somewhat confusing as you then have to guess form the prototypes.
- quite a few of the functions don't seem to do any error checking, what
happens if yubikey_hex_decode hits a non-hex character? It seems
undefined.
These are a few of my experiences using it so far; it works, but is
unnecessarily quirky to use. I realise I'm probably one of the first
outside of yubico to write an application using it and the API might
feel a bit raw because of that. Hopefully my comments are useful for
the next API version. I'm happy to help out where I can, if you're
interested.
Comments are of course welcome; I'm subscribed to the list and happy to
discuss my experiences.
Regards,
--
Tollef Fog Heen
UNIX is user friendly, it's just picky about who its friends are
Simon Josefsson
> Hi,
>
> I'm in the middle of writing a yubikey validation server in C, because I
> for various reasons don't want one in Java or PHP.
Hi Tollef. Cool! I have considered that for performance reasons, but
the Java/PHP was fast enough that I never bothered.
It is within the scope of the yubico-c project to implement more of the
validation server logic, so if you want to donate some of your code to
it, that would be splendid. The problematic part is the database part
during validation, but I think the library could simply allow use of a
callback into the application to fetch the parameters. I wouldn't want
libyubikey (or libyubikey2, see below) link to any database library.
Also interesting would be to add APIs to _create_ OTPs from information
plus an AES key, which is useful for testing purposes.
> I thought I'd write some bits about my experiences using libyubikey.
> The list is not too structured at the moment, I'm sorry about that,
> but figured it would be better to send it than to forget. :-)
Yes, thank you very much for sending it, feedback like this is very
valuable.
> - It's confusing that yubikey_token_t is not a real type, but just a
> pointer to a type (yubikey_token_st)
Yeah, maybe the struct should be yubikey_token_t and the APIs use
'yubikey_token_t *' variables.
> - yubikey_parse takes in one a modhex encoded token and an unencoded AES
> key. This is somewhat confusing too. It also requires you to strip
> the public uid from the front of the token rather than just doing it
> for you.
Perhaps a utility function could be added for this, I'd prefer
yubikey_parse to reject anything that is not fully conforming rather
than stripping bits of.
> - yubikey_crc_ok_p(tok) does not actually take in a token, it takes in a
> uint8_t, requiring you to do odd stuff like:
> yubikey_crc_ok_p((uint8_t*)&token). (token is a yubikey_token_st
> here.) IMO any API which forces you to use casting when doing normal,
> routine operations needs fixing.
Yup.
> - the yubikey_token_t struct is a bit on the low-level side -- why is
> the timestamp split across two fields, and why is the «key triggered
> by caps lock» bit of the session counter not moved into its own field?
The intention is that the struct should map directly to the decrypted
contents, memory-wise. Is there a portable uint24_t? I don't think
there are portable uint7_t + uint1_t types.
> - the use field of the token structure seems a bit underspecified, the
> comment says: «Number of times used within session + activation
> flags.», but I'm not able to find typedefs for any activation flags
> (or similar) in the header file.
The activation flag is the capslock stuff, which has been completely
removed from modern yubikey's. So ideally the comments could be
removed.
> - functions don't say whether they allocate memory or not, which is
> somewhat confusing as you then have to guess form the prototypes.
Right.
> - quite a few of the functions don't seem to do any error checking, what
> happens if yubikey_hex_decode hits a non-hex character? It seems
> undefined.
We noticed this in yubikey-personalization too, and my workaround to
that was to add yubikey_modhex_p and yubikey_hex_p to permit checking.
> These are a few of my experiences using it so far; it works, but is
> unnecessarily quirky to use. I realise I'm probably one of the first
> outside of yubico to write an application using it and the API might
> feel a bit raw because of that. Hopefully my comments are useful for
> the next API version. I'm happy to help out where I can, if you're
> interested.
>
> Comments are of course welcome; I'm subscribed to the list and happy to
> discuss my experiences.
Perhaps we could use your comments as a basis to develop a new header
file? I think the current API/ABI needs to stay forever, so we should
probably add it as yubikey2.h and libyubikey2 or similar (other ideas
welcome) to avoid causing problems for people already using libyubikey.
If you want to work on this, you have my support.
During review of the new header file, I'm sure we can come up with other
improvements as well.
For example, it would be nice to (optionally!) link to some other
library for AES, to avoid code duplication, which makes sense for
distributions like Debian.
I've given you commit access to yubico-c, but please don't make any
backwards incompatible changes.
/Simon
Tollef Fog Heen
Hiya,
| Tollef Fog Heen <tfh...@err.no> writes:
|
| > Hi,
| >
| > I'm in the middle of writing a yubikey validation server in C, because I
| > for various reasons don't want one in Java or PHP.
|
| Hi Tollef. Cool! I have considered that for performance reasons, but
| the Java/PHP was fast enough that I never bothered.
I don't like systems software in java, and PHP for something security
sensitive does not give me a good feeling, so C it is.
| It is within the scope of the yubico-c project to implement more of the
| validation server logic, so if you want to donate some of your code to
| it, that would be splendid.
My code is currently GPLv2 due to external libraries, but I'd be fine
with relicencing the bits I have written myself.
I don't think too much of the validation server should be part of
libyubikey itself, the flow is basically something like:
/* Parse query string, grab id, otp_mh and h (optional) */
/* Do query to grab shared secret, we need this later anyway */
/* If h exists, verify. */
/* Validate OTP */
/* Find public uid, if possible */
(speaking of which, if there's no public ID, what should I do? Reject
the request or try to find out what UID the key belongs to? I'm tempted
to just reject)
/* Update status, if appropriate */
/* Generate response, sign it */
Currently this means we have to be able to parse a query string, encode
and decode base64, get and set data in the database and do HMAC-SHA1
operations. In addition to this we must speak HTTP (I'm using
libmicrohttpd, which is LGPL).
Of those, having base64 and hmac-sha1 in libyubikey itself makes some
sense. Parsing the query string not so much and certainly not the
database operations.
| The problematic part is the database part during validation, but I
| think the library could simply allow use of a callback into the
| application to fetch the parameters. I wouldn't want libyubikey (or
| libyubikey2, see below) link to any database library.
Agreed on this, no linking to any database library.
| Also interesting would be to add APIs to _create_ OTPs from information
| plus an AES key, which is useful for testing purposes.
Indeed, and it would make it easier to have a useful test suite.
| > - It's confusing that yubikey_token_t is not a real type, but just a
| > pointer to a type (yubikey_token_st)
|
| Yeah, maybe the struct should be yubikey_token_t and the APIs use
| 'yubikey_token_t *' variables.
Either that or have the type be opaque and getters/setters and functions
to allocate and free the struct. Either works.
| > - yubikey_parse takes in one a modhex encoded token and an unencoded AES
| > key. This is somewhat confusing too. It also requires you to strip
| > the public uid from the front of the token rather than just doing it
| > for you.
|
| Perhaps a utility function could be added for this, I'd prefer
| yubikey_parse to reject anything that is not fully conforming rather
| than stripping bits of.
This is actually two points: Either both args should be encoded or none
of them should be encoded. I don't really care which we go with as long
as it's symmetrical. Ideally, it should be the same across the library:
«OTP tokens are always modhex encoded and AES keys are always encoded as
hex strings».
I think it should return an error if it fails to validate too. I assume
you're ok with that?
| > - the yubikey_token_t struct is a bit on the low-level side -- why is
| > the timestamp split across two fields, and why is the «key triggered
| > by caps lock» bit of the session counter not moved into its own field?
|
| The intention is that the struct should map directly to the decrypted
| contents, memory-wise. Is there a portable uint24_t? I don't think
| there are portable uint7_t + uint1_t types.
I can see the point of having that as a low-level API, but for an
application programmer, why is it interesting? I'd just go with a 32
bit int where the upper 8 bits are always zero, and for session counter
I'd go with a full 8 bit field and waste a full byte on the caps lock
field. We're probably never going to load two hundred million structs
into memory at once anyway, so micro-optimising for memory usage doesn't
make sense. (We could use a bitfield for the single bit, but that's
just going to be padded by the compiler, so I wouldn't bother.)
| > - the use field of the token structure seems a bit underspecified, the
| > comment says: «Number of times used within session + activation
| > flags.», but I'm not able to find typedefs for any activation flags
| > (or similar) in the header file.
|
| The activation flag is the capslock stuff, which has been completely
| removed from modern yubikey's. So ideally the comments could be
| removed.
ok. I got confused over the «flags» bit (plural).
| > - quite a few of the functions don't seem to do any error checking, what
| > happens if yubikey_hex_decode hits a non-hex character? It seems
| > undefined.
|
| We noticed this in yubikey-personalization too, and my workaround to
| that was to add yubikey_modhex_p and yubikey_hex_p to permit checking.
I think the APIs should either return errors or if you want to go with
the _p variants: put in some asserts to make sure what we get actually
is good.
Personally, I'd just go for the former, handling errors is part of the
normal flow when writing C.
| > Comments are of course welcome; I'm subscribed to the list and happy to
| > discuss my experiences.
|
| Perhaps we could use your comments as a basis to develop a new header
| file?
yes, please. Should I just start writing something on the wiki and we
can add bits and discuss them on this list when there are questions that
need fleshing out? Wikis are good for sharing a common state, but not
for discussions. Or we can just have some header files in the svn repo
marked as «NOT STABLE, DO NOT USE».
| I think the current API/ABI needs to stay forever, so we should
| probably add it as yubikey2.h and libyubikey2 or similar (other ideas
| welcome) to avoid causing problems for people already using libyubikey.
| If you want to work on this, you have my support.
Forever is a long time. If you really want to have co-installable APIs
and such, we can do that, sure.
| During review of the new header file, I'm sure we can come up with other
| improvements as well.
Sure. :-)
| For example, it would be nice to (optionally!) link to some other
| library for AES, to avoid code duplication, which makes sense for
| distributions like Debian.
Might make sense, yes. AES itself isn't that big though, so if we need
to keep it for situations where the system doesn't have a library, it
might make sense to just keep all the code internally.
| I've given you commit access to yubico-c, but please don't make any
| backwards incompatible changes.
Thanks, and I won't. :-)