Skip to content
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

Bring back more ciphersuites #279

Merged
merged 10 commits into from Mar 5, 2020
Merged

Bring back more ciphersuites #279

merged 10 commits into from Mar 5, 2020

Conversation

raphaelrobert
Copy link
Member

@raphaelrobert raphaelrobert commented Dec 28, 2019

No description provided.

Copy link
Collaborator

@Bren2010 Bren2010 left a comment

The file no longer compiles, not sure why

draft-ietf-mls-protocol.md Show resolved Hide resolved
draft-ietf-mls-protocol.md Show resolved Hide resolved
draft-ietf-mls-protocol.md Outdated Show resolved Hide resolved
draft-ietf-mls-protocol.md Show resolved Hide resolved
draft-ietf-mls-protocol.md Outdated Show resolved Hide resolved
draft-ietf-mls-protocol.md Show resolved Hide resolved
draft-ietf-mls-protocol.md Show resolved Hide resolved
| Component | Contents |
|:----------|:---------|
| MLS | The string "MLS" followed by the major and minor version, e.g. "MLS10" |
| LVL | The security level |
Copy link
Collaborator

@bifurcation bifurcation Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem all that useful, I would drop it.

Copy link
Member Author

@raphaelrobert raphaelrobert Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to give you a rough idea of the overall security level for informational purposes. Maybe @beurdouche can elaborate more on that.

draft-ietf-mls-protocol.md Outdated Show resolved Hide resolved
draft-ietf-mls-protocol.md Show resolved Hide resolved

* Hash function: SHA-256
* AEAD: AES-128-GCM
The ciphersuites are defined in section {{MLS Ciphersuites}}.
Copy link
Collaborator

@bifurcation bifurcation Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need a bit more description here, just to say what a ciphersuite indicates, namely:

  • Parameters for an instance of HPKE
  • A KDF to be used
  • As a consequence, a Derive-Key-Pair function, as defined below.

Copy link
Collaborator

@bifurcation bifurcation Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, does {{MLS Ciphersuites}} actually work? Might need to be {{mls-ciphersuites}}.

Copy link
Member Author

@raphaelrobert raphaelrobert Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. That's why I wanted to leave "HPKE" in the name of the ciphersuite
  2. I guess that should then go in line 809
  3. That's what we have in the "Diffie-Hellmann groups section"

Regarding the notation for hyperlinks, I'll take your word for it

Copy link
Contributor

@seanturner seanturner Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this in mine. The anchor is {#anchor} and then you link in text with {{anchor}}.


When HPKE is used with this ciphersuite, it uses the following
algorithms:
### Notes on Diffie-Hellman groups
Copy link
Collaborator

@bifurcation bifurcation Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra section heading doesn't seem necessary.

Copy link
Contributor

@seanturner seanturner Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I blew it away in mine.

* Hash function: SHA-256
* AEAD: AES-128-GCM
Given an octet string X, the private key produced by the
Derive-Key-Pair operation is SHA-512(X) truncated to 448 bits (Recall that any 56-octet
Copy link
Collaborator

@bifurcation bifurcation Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use HKDF instead of SHA directly? In other words, make node secrets variable-length, where the length is determined by the KEM in use.

  • P256, X25519: 32 bytes
  • X448: 56 bytes
  • P521: 66 bytes

As a bonus, that would save a couple of hash invocations.

Copy link
Member Author

@raphaelrobert raphaelrobert Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is a specific reasons not to do it (cc @beurdouche), I think it would be great to harmonise this. Having different hash algorithms in here makes no sense to me if it can be avoided.

@bifurcation
Copy link
Collaborator

@bifurcation bifurcation commented Jan 3, 2020

It seems like we have a few separable issues here:

  1. Whether we require a single signature scheme for the whole group
  2. Whether signature schemes are included in the ciphersuite
  3. Whether we have an MTI ciphersuite / signature algorithm
  4. What new ciphersuites should be defined, and what algorithms should go in them

Let's discuss this at the interim and maybe break this PR into a few bits

Copy link
Collaborator

@bifurcation bifurcation left a comment

This seems to accurately reflect the consensus we had at the last interim, and there's nothing I can't live with otherwise.

I will probably file a follow-up to convert the Derive-Key-Pair operations to use HKDF directly, as discussed, avoiding the need for intermediate node_secret values or truncation of SHA-512.

@bifurcation bifurcation added today! (?) and removed editorial ready for review (by editors) labels Feb 4, 2020
Copy link
Collaborator

@Bren2010 Bren2010 left a comment

There are some items missing that I believe we agreed on at the last in-person interim:

  • It's not clear from reading the PR that the signature scheme is now fixed per-group.
    • Need to verify that CIKs correspond to current suite and express support for all algorithms currently used.
  • Recommendation that we be conservative when adding new suites in the future.
  • Justification for why the current suites are minimal.

@bifurcation bifurcation added ready to merge and removed today! (?) labels Feb 6, 2020
@seanturner
Copy link
Contributor

@seanturner seanturner commented Feb 13, 2020

Note that specification required registries imply that there is a designated expert pool. I will add some text to create one. The text is very similar to the TLS DE pool text from RFC8447 with some tweaks that, hopefully, address the misunderstandings uncovered during non-standards track registry requests.

Note the process is: (assuming that the IESG eventually approves this draft) When the IESG approves this draft, the responsible AD assigns some DEs (like 2-3 suggested by the WG chairs). These DEs are in charge of all registrations regardless of whether the draft comes from the WG or elsewhere as all requests go to the MLS DEs mailing list.

X25519_SHA256_AES128GCM(0x0001),
(0xFFFF)
} CipherSuite;
The signature algorithm specified in the ciphersuite is the mandatory algorithm to be used for the signatutes in MLSPlaintext and the tree signatures. It can be different from the signature algorithm specified in credential fielf of ClientInitKeys.
Copy link
Collaborator

@Bren2010 Bren2010 Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please wrap this at 80-ish chars
  • "field"
  • Can it really be different?

Derive-Key-Pair operation is SHA-256(X). (Recall that any 32-octet
string is a valid Curve25519 private key.) The corresponding public
Derive-Key-Pair operation is SHA-256(X) (Recall that any 32-octet
string is a valid X25519 private key). The corresponding public
Copy link
Collaborator

@Bren2010 Bren2010 Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The punctuation was correct before
Alternatively, lower-case r in "Recall"

CipherSuite MLS_LVL_KEM_AEAD_HASH_SIG = VALUE;
~~~

Where VALUE is a is represented as two 8bit octets:
Copy link
Collaborator

@Bren2010 Bren2010 Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VALUE is represented
8-bit

| KEM | The KEM algorithm used for HPKE in TreeKEM group operations |
| AEAD | The AEAD algorithm used for HPKE and message protection |
| HASH | The hash algorithm used for HPKE and the MLS KDF |
| SIG | The Signature algorithm used for message authentication |
Copy link
Collaborator

@Bren2010 Bren2010 Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lower-case s in "Signature"

The initial contents for this registry are as follows:
The mandatory-to-implement ciphersuite for MLS 1.0 is
`MLS10\_128\_HPKE25519\_AES128GCM\_SHA256\_Ed25519` which is using
Curve25519, HKDF over SHA2-256 and AES-128-GCM for HPKE,
Copy link
Collaborator

@Bren2010 Bren2010 Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HKDF over SHA-256, AES-128-GCM for HPKE,

`MLS10\_128\_HPKE25519\_AES128GCM\_SHA256\_Ed25519` which is using
Curve25519, HKDF over SHA2-256 and AES-128-GCM for HPKE,
and AES-128-GCM with Ed25519 for symmetric encryption and
signatures.
Copy link
Collaborator

@Bren2010 Bren2010 Feb 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"message encryption and authentication"

@bifurcation bifurcation merged commit 74380c5 into master Mar 5, 2020
0 of 5 checks passed
@raphaelrobert raphaelrobert deleted the raphael_ciphersuites branch Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants