Go: wiki: clarify Assembly Policy for crypto

Created on 10 Oct 2018  路  8Comments  路  Source: golang/go

Current Assembly Policy establish general guidelines for inclusion of assembly implementations. However there is some confusion about it application to crypto.
I think we should clarify the following questions:

  • Are assembly implementations of new (to go) modes/hashes/block-ciphers allowed at all?
  • If they are allowed, we should have a list of deprecated stuff, that is intended to be slow e. g. rc4.
  • Are there any additional requirements for testing crypto asm?
  • Can I (or anyone without great background in crypto) +2 straightforward cases e. g. https://go-review.googlesource.com/c/go/+/125316 ?
  • If we are not going to accept new assembly implemntaions, than we must do something with CLs sent before creation of assembly policy, that are still in review, e. g. https://go-review.googlesource.com/c/go/+/109135
Documentation

Most helpful comment

Current list of open CLs tagged crypto-assembly: https://go-review.googlesource.com/q/hashtag:%22crypto-assembly%22+(status:open)

All 8 comments

/cc @FiloSottile @aead

/cc @mundaym @Quasilyte

I agree that the assembly policy is a bit vague and think that there should be a bit more guidance about assembly code. So I'll try to answer some questions from the perspective of an external contributor.

Are assembly implementations of new (to go) modes/hashes/block-ciphers allowed at all?

I think prohibiting assembly implementations in general is probably not a good idea. However, the bar for new assembly code should be quite high. Personally I think there should be evidence that:

  • the primitive is commonly used (e.g. as (important) part of TLS)
  • the assembly code improves performance significantly
  • the primitive is often part of a performance-critical path

The primitive should also be standardized as an RFC (or similar).
Additionally if the primitive is not "state-of-the-art" anymore it should be less likely to accept new assembly implementations. No new assembly code should be accepted for a broken primitive - except there is/are very strong arguments to do so.

For existing assembly code I think new code can also be added if:

  • the amount of assembler code is reduced (and ideally the performance improves, too) (e.g. golang/x/crypto/poly1305
  • the amount of assembler code is quite small compared to its performance improvements (already mentioned by the AssemblyPolicy)

Again we should not try to optimize "legacy"/out-dated primitives if not really needed.

If they are allowed, we should have a list of deprecated stuff, that is intended to be slow e. g. rc4.

Agree here. There should be a list of packages for which assembly code won't get accepted - e.g. because existing asm code was removed...

Are there any additional requirements for testing crypto asm?

Should it be possible to test all crypto (asm) code against BorringSSL (dev.boringcrypto)? @FiloSottile
IMO there should be (probabilistic) tests against the reference Go implementation additionally to
predefined test vectors.

the amount of assembler code is quite small compared to its performance improvements (already mentioned by the AssemblyPolicy)

The AssemblyPolicy should clarify on how performance gain is small (10% ? 30%?)

The problem is the stdlib doesn't comes with the interfaces (mainly net/http/encoding) for the users to choose the libs them want. If the users wants to improve the performance they have to build up the entire stdlib dependency path all by themselves or just endure the bad performance of stdlib.
IMHO we should at least maintain all the stdlib related (not legacy) asm code.

The AssemblyPolicy should clarify on how performance gain is small (10% ? 30%?)

Hm, I don't think just defining a minimum improvement is enough - leaving aside I don't know what the concrete number would be. For example it's probably not worth to accept a 20% improvement if this would add 2-3K lines of assembly. So it's probably more about the ratio of code (complexity) to speed up. However I don't know how this can be formalized.

IMHO we should at least maintain all the stdlib related (not legacy) asm code.

In general agree here. But I don't think it's worth to add new code if the requirements mentioned above aren't fulfilled.

The problem is the stdlib doesn't comes with the interfaces (mainly net/http/encoding) for the users to choose the libs them want.

That sounds like a separate proposal to file.

Current list of open CLs tagged crypto-assembly: https://go-review.googlesource.com/q/hashtag:%22crypto-assembly%22+(status:open)

Folding into #37168. These are all good questions that I'd like us to provide clear answers for (except maybe an explicit performance ration at which we bring in assembly, because I think it will always be a judgement call based on a number of hard to capture factors).

Was this page helpful?
0 / 5 - 0 ratings