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

Modify the authorized key signature process when the pre-rotation fea… #129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bj-ms
Copy link
Contributor

@bj-ms bj-ms commented Nov 14, 2024

TLDR: Modify the authorized key signature process when the pre-rotation feature is enabled. Commited keys must be used directly in the next log entry.

Hi everyone,

During IIW 39b, I talked to Stephen Curran and Patrick St-Louis about a potential issue I see with key pre-rotation as it is stated now in the specification. There is a case that an attacker could create a malicious version of the DID Document because the commited updateKey is only enforced AFTER the publication of the log entry. Normally in the key pre-rotation concept, if you commit to nextKeyHashes, you must use it to sign the next log entry.

But the security risk is extremely low right now because you need to fulfill those two conditions:

  • The Attacker must have compromised the current updateKey (theft or PQC)
  • The Attacker must known the public key behind the nextKeyHashes (theft or data leak)

Even if the risk is low, i would propose to fix the pre-rotation concept now. better safe than sorry :)

We will need to also adapt the examples and I would argue to jump to version 0.5, as it is a breaking change.

…ture is enabled. Commited keys must be used directly in the next log entry

Signed-off-by: Michel Sahli <michel.sahli@bj.admin.ch>
@brianorwhatever
Copy link
Contributor

The text you have added is how I was originally thinking about it. Maybe I had made assumptions about the specification that weren't true. The current implementation in trustdidweb-ts requires ANY changes to the DID to be authorized by the currently active updateKeys which is what it seems like this PR is adding for pre-rotation keys.. am I missing something?

@bj-ms
Copy link
Contributor Author

bj-ms commented Nov 18, 2024

The text you have added is how I was originally thinking about it. Maybe I had made assumptions about the specification that weren't true. The current implementation in trustdidweb-ts requires ANY changes to the DID to be authorized by the currently active updateKeys which is what it seems like this PR is adding for pre-rotation keys.. am I missing something?

If you want a secure linked list of hashes, you will need to sign a new did log entry with the previous updateKey. If you don't "link" the entries with the previous entry, anyone could just create a new did log entry with a new updateKey and sign the entry with that same key.

The difference with pre-rotation keys, is that you commit to a new key with NextKeyHash. When you create a new did log entry, you need to sign with the new commited updateKey to use the full potential of pre-roration keys.

tldr: without pre-roration keys, you need to link with the signature of the previous updateKey. If you use pre-rotation keys, you link the did log entries with the nextKeyHashes and must use the current commited updateKey to generate the signature.

@swcurran
Copy link
Collaborator

That is very confusing — I didn’t really understand the change based on the PR. I think I now get the idea (questions, summary below), but I think it will take a bigger update to the specification to make it clear.

I think the core idea of what you are saying is that because of the pre-commitment, there is no need to wait until after “updateKeys” published to use them — we can sign the record with a key from the list in the current record.

I don’t think it really reduces the threat significantly. From what you had above, we now have:

  • The Attacker must known the public key behind the nextKeyHashes (theft or data leak)
  • The Attacker must known the public key behind the nextKeyHashes key (theft or PQC)

That is only a slight variation on the risk that is motivating the change.

Is the following an accurate description of your proposal:

No Prerotation:

  • First entry is signed by a key in the first record “updateKeys” list.
  • Subsequent entries are signed by a key in the active “updateKeys” list.
  • An “updateKeys” list is active after the log entry it is in is written, through to when another “updateKeys” record is written (e.g., the new list becomes active).

Preotation:

  • There must by an “updateKeys” and “nextKeys” list in every record.
  • All entries are signed by a key in the “updateKeys” list of the current record.
  • All of the keys in each “updateKeys” list MUST be in the active “nextKeys” list.
  • A “nextKeys” list is active after the log entry it is in is written, through to when another “nextKeys” record is written (e.g., the new list becomes active).

Question — In your scheme, is it a requirement that the “updateKeys” and “nextKeys” list are in every record (first bullet above), or can multiple records be signed by keys in the same “updateKeys” list?

@bj-ms
Copy link
Contributor Author

bj-ms commented Nov 19, 2024

Thanks for clarifying my description, yes it is accurate.

For your question, most other technical specification of pre-rotation I have seen forces the concept of updateKeys and nextKeys in each record. It is more secure because it closes the gap where attackers could compromise the current updateKey and sign a new log entry.

The main protection that people want from pre-rotation keys is that even if the updatekey is compromised, the attacker cannot create a new log entry. And since the nextKeyHash is a hash, it is also more resistant to theorical PQC attacks.

I would recommend to force updateKey and nextKey in each record for security and simplicity.

@bj-ms
Copy link
Contributor Author

bj-ms commented Nov 22, 2024

Decided to adapt the pre-rotation text with the global understanding discussed during the last meeting: https://hackmd.io/k4cIK9vQSlaeg2pdHE51IQ.

@brianorwhatever
Copy link
Contributor

I am still struggling to grok this so I have made diagrams to understand the process proposed AFTER this change has been accepted. Please correct me if I'm misunderstanding things. Of note: In the third image you see what I was concerned about on the call which is the unused key c which is likely not an issue..
did_webh no presentation
did_webh prerotation 1
did_webh prerotation 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants