Skip to content

wiki: clarify Assembly Policy for crypto #28126

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

Closed
TocarIP opened this issue Oct 10, 2018 · 8 comments
Closed

wiki: clarify Assembly Policy for crypto #28126

TocarIP opened this issue Oct 10, 2018 · 8 comments
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge
Milestone

Comments

@TocarIP
Copy link
Contributor

TocarIP commented Oct 10, 2018

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
@TocarIP
Copy link
Contributor Author

TocarIP commented Oct 10, 2018

/cc @FiloSottile @aead

@mvdan
Copy link
Member

mvdan commented Oct 10, 2018

/cc @mundaym @quasilyte

@aead
Copy link
Contributor

aead commented Oct 10, 2018

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.

@mengzhuo
Copy link
Contributor

mengzhuo commented Oct 14, 2018

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.

@aead
Copy link
Contributor

aead commented Oct 15, 2018

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.

@bcmills bcmills added the Documentation Issues describing a change to documentation. label Oct 23, 2018
@bcmills
Copy link
Contributor

bcmills commented Oct 23, 2018

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.

@dgryski
Copy link
Contributor

dgryski commented Dec 4, 2018

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

@FiloSottile
Copy link
Contributor

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).

@golang golang locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

8 participants