924390 - Allow performing low-level crypto operations without using SECItem structures

Status: RESOLVED WONTFIX

(NSS :: Libraries, enhancement, P5)

Description

[Miloslav Trmač]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130917102605

Steps to reproduce:

While the SECItem structure is convenient for passing a block of data through multiple layers of abstraction, or for communicating between the low-level crypto routines and the ASN.1 encoders/decoders, for applications manipulating raw data it is rather inconvenient, e.g.
> SECItem src_item, dest_item;
> src_item.data = (void *)src;
> src_item.len = src_size;
> dest_item.data = dest;
> dest_item.len = dest_buf_size;
> if (PK11_Sign (key, &dest_item, &src_item) != SECSuccess) /* error handling */
> dest_value_size = dest_item.len
is much more code than a hypothetical
> if (PK11_Sign (key, dest_buf, dest_buf_size, &dest_value_size, src, src_size) != SECSuccess) /* error handling */

So, please consider reviewing the raw or raw-ish crypto interfaces (e.g. pk11pub.h, cryptohi.h) for calls where providing an alternative/wrapper that refers to data directly instead of through SECItems would make the callers simpler.

Comment 1

[Miloslav Trmač]

Related: #924396 "easier to use low-level crypto", e.g. hiding the existence of slots.

Comment 2

[Kai Engert (:KaiE:)]

It could also be written as:
  SECItem src_item = {0, src, src_size};
  SECItem dest_item = {0, dest, dest_buf_size};
  if (PK11_Sign (key, &dest_item, &src_item) != SECSuccess) /* error handling */
  dest_value_size = dest_item.len:

which isn't that much longer. One could argue that this is even more readable. And it's less risky to accidentally mix the order of parameters, if there aren't a large list of parameters.

I'm not yet convinced this would be a big win in usability.

If our examples listed this way of use, thereby inspiring new users to use this shorter form, it might be sufficient?

And if you need temporary wrapper objects, and want them to be located immediately next to your function call, it's always possible to add a { } scope.

  code...
  {
    SECItem...
    if (function())..
  }

Comment 3

[Miloslav Trmač]

(In reply to Kai Engert (:kaie) from comment #2)
> It could also be written as:
>   SECItem src_item = {0, src, src_size};
>   SECItem dest_item = {0, dest, dest_buf_size};
>   if (PK11_Sign (key, &dest_item, &src_item) != SECSuccess) /* error
> handling */
>   dest_value_size = dest_item.len:

That's still 3 extra lines and 2 extra variables (and, strictly speaking, the layout of SECItemStr doesn't seem to be documented).

> I'm not yet convinced this would be a big win in usability.
Right, this isn't a matter of yes/no; it's a balance between ease of use, maintenance effort, and size of API (=> maintenance effort and possible confusion).  

I'm convinced (based also on comments from real users) that overall the low-level crypto API is more difficult to use than necessary; I'm not at all certain that making only this change would be the appropriate fix.  Perhaps the split between this bug and the more vague bug #924396 wasn't such a good idea.