Opened 5 years ago

Closed 4 years ago

#18875 closed enhancement (fixed)

Consider replacing RelayNetworkStatusVote's getDirectorySignatures() with getDirectorySignature()

Reported by: karsten Owned by: karsten
Priority: Medium Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: nickm, iwakeh Actual Points:
Parent ID: #19398 Points:
Reviewer: Sponsor:

Description

(nickm, there's a dir-spec-related question below that you might have an answer to.)

dir-spec.txt says:

   The signature contains the following item, which appears Exactly Once
   for a vote, and At Least Once for a consensus.

    "directory-signature" [SP Algorithm] SP identity SP signing-key-digest
        NL Signature

Is that still true, or can there be multiple such lines for different algorithms?

Assuming this is still true, the following method that we're providing in RelayNetworkStatusVote is confusing:

/* Return directory signatures. */
public SortedMap<String, DirectorySignature> getDirectorySignatures();

It's confusing to return a map of directory signatures when there can only be one such signature. We should instead make sure that there's exactly one such signature and then return it in a new method:

/* Return directory signature. */
public DirectorySignature getDirectorySignature();

We should also deprecate the existing method and suggest to use the new method instead.

But let's first find out whether there can really only be one signature per vote. And if there can be multiple signatures for different digest algorithms, we might have to think harder how to provide that in a method.

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by tomlurge

RelayNetworkStatusVote is used for both Votes and Consensuses. For Consensuses returning a Map is the right thing to do. So don't deprecate getDirectorySignatures() altogether but maybe

  • add getDirectorySignature() to RelayNetworkStatusVote
  • or make sure that for a Vote the first or last (and preferably only) entry in the Map is the DirSig
  • or change RelayNetworkStatusVote into an interface RelayNetworkStatus with RelayNetworkStatusVote and RelayNetworkStatusConsensus as implementations (with the respective methods)

Sorry, that was just wrong.

Last edited 5 years ago by tomlurge (previous) (diff)

comment:2 Changed 4 years ago by teor

I think the underlying question here is:
"What will happen when we stop using SHA1/RSA_PKCS1_PADDING for our consensus digests?"

I would imagine we'll have to sign both SHA1/RSA_PKCS1_PADDING and SHA256/ED25519(?) for a while.

I also have a related question:
What is the "String" key in the current metrics-lib getDirectorySignatures() map?
How does it handle signatures from legacy keys?

I'd suggest passing the algorithm / identity / signing key digest to the function (if they're not already implicit as part of the RelayNetworkStatusVote object).
That way, you can return the appropriate signature.
Perhaps it's worth having a form of the function with sensible defaults, like getDirectorySignatureSHA1RSA(), which would get the SHA1/RSA signature from the most recent signing key for that authority.

comment:3 in reply to:  2 Changed 4 years ago by karsten

Replying to teor:

Thanks for your thoughts here!

I think the underlying question here is:
"What will happen when we stop using SHA1/RSA_PKCS1_PADDING for our consensus digests?"

I would imagine we'll have to sign both SHA1/RSA_PKCS1_PADDING and SHA256/ED25519(?) for a while.

Probably, yes. I mean, we could also teach all fellow directory authorities how to verify the new signature scheme and then switch, but it seems easier to just include two signatures for a while. It would violate the "Exactly Once" part in dir-spec.txt, but I guess that's easy to change.

I'd say, for metrics-lib, let's assume it's at least possible that votes contain more than one signature, even if that has not happened so far.

I also have a related question:
What is the "String" key in the current metrics-lib getDirectorySignatures() map?

From the latest Javadocs, that string is "the SHA-1 digest of the authority's identity key in the version 3 directory protocol, encoded as 40 upper-case hexadecimal characters."

Maybe the better answer is: let's get rid of that string, because it's confusing and is already contained in DirectorySignature, and turn the map into a list. (See suggestion below.)

How does it handle signatures from legacy keys?

Uhm, fine question. IIUC, votes only contain signatures made by the authority's non-legacy key, and consensuses contain both legacy and non-legacy signatures. So, for this ticket where we primarily consider votes, this doesn't matter. And consensuses simply contain more signatures than there were authorities contributing to the consensus.

I'd suggest passing the algorithm / identity / signing key digest to the function (if they're not already implicit as part of the RelayNetworkStatusVote object).
That way, you can return the appropriate signature.
Perhaps it's worth having a form of the function with sensible defaults, like getDirectorySignatureSHA1RSA(), which would get the SHA1/RSA signature from the most recent signing key for that authority.

I think the simpler approach is to return a list of all signatures which contain algorithm, identity and signing key digest, and let the application skip over all signatures it doesn't care about. That would be public List<DirectorySignature> getSignatures(), both in consensuses and in votes.

Again, thanks for your feedback!

comment:4 Changed 4 years ago by karsten

Parent ID: #19398

comment:5 Changed 4 years ago by karsten

Cc: iwakeh added
Status: newneeds_review

Please review my branch task-18875 with a possible fix.

comment:6 Changed 4 years ago by iwakeh

Seems fine, only I would suggest to make the algorithm types public constants.
Found here and here.

comment:7 Changed 4 years ago by karsten

Ah, I just pushed a fix that turns "sha1" into a constant and then re-read your comment. What do you mean by public constant? We don't yet know what other algorithms will be used in the future, and we shouldn't have to add a new constant whenever tor adds a new algorithm. Or what did you mean?

As always, thanks for the review!

comment:8 Changed 4 years ago by iwakeh

Assuming we don't verify the algorithm strings then your implementation is fine.

I have another question:
The dir-spec, line 2044ff states that currently there can be two types of algorithms. With the reasoning above (comment:3) won't there be the situation that some provide sha1, some sha256, others both? If so, the signing key digest would be null for the non-default version, even though there is a signing key, i.e. collector/metrics-lib only provides the signing key digest of the default algorithm, is that intended?

comment:9 in reply to:  8 Changed 4 years ago by karsten

Replying to iwakeh:

Assuming we don't verify the algorithm strings then your implementation is fine.

Great! Thanks for looking!

I have another question:
The dir-spec, line 2044ff states that currently there can be two types of algorithms. With the reasoning above (comment:3) won't there be the situation that some provide sha1, some sha256, others both? If so, the signing key digest would be null for the non-default version, even though there is a signing key, i.e. collector/metrics-lib only provides the signing key digest of the default algorithm, is that intended?

Huh, very good point. And now that I look closer at the getSigningKeyDigest() method, I'd say let's deprecate and later remove it. It's a convenience method that, however, makes the implicit assumption on the descriptor that there's at least one signature made with "sha1". We're not making any data inaccessible by taking out that method, but we're removing a possible source of confusion. Please take another look at my updated branch. Thanks!

comment:10 Changed 4 years ago by iwakeh

Status: needs_reviewmerge_ready

looks good.

comment:11 Changed 4 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

Thanks! Squashed and merged. Closing.

Note: See TracTickets for help on using tickets.