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

write some words about lease renewal secrets #1118

Conversation

exarkun
Copy link
Member

@exarkun exarkun commented Sep 2, 2021

@exarkun exarkun marked this pull request as ready for review September 3, 2021 13:18
@exarkun exarkun requested a review from a team September 3, 2021 13:18
@coveralls
Copy link
Collaborator

coveralls commented Sep 3, 2021

Coverage Status

Coverage remained the same at 95.49% when pulling ee22430 on LeastAuthority:3774.lease-renewal-secret-construction-docs into 1819e08 on tahoe-lafs:master.

Copy link
Collaborator

@itamarst itamarst left a comment

Choose a reason for hiding this comment

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

Thank you, probably needs a bit more clarification.

* the string itself
* ``","``

* The **sha256d digest** is the **sha256 digest** of the **sha256 digest** of a string.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole section would be a lot clearer if there was also some code included to demonstrate (with type annotations).

Copy link
Member Author

Choose a reason for hiding this comment

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

something like this?

"""                                                                                                                                                                                           
This is a reference implementation of the lease renewal secret derivation                                                                                                                     
protocol in use by Tahoe-LAFS clients as of 1.16.0.                                                                                                                                           
"""

from base64 import (
    b32encode,
)

from allmydata.util.hashutil import (
    tagged_hash,
    tagged_pair_hash,
)


def derive_renewal_secret(lease_secret: bytes, storage_index: bytes, tubid: bytes) -> bytes:
    assert len(lease_secret) == 32
    assert len(storage_index) == 20
    assert len(tubid) == 20

    bucket_renewal_tag = b"allmydata_bucket_renewal_secret_v1"
    file_renewal_tag = b"allmydata_file_renewal_secret_v1"
    client_renewal_tag = b"allmydata_client_renewal_secret_v1"

    client_renewal_secret = tagged_hash(lease_secret, client_renewal_tag)
    file_renewal_secret = tagged_pair_hash(
        file_renewal_tag,
        client_renewal_secret,
        storage_index,
    )
    peer_id = b32encode(tubid).lower().strip(b"=")

    return tagged_pair_hash(bucket_renewal_tag, file_renewal_secret, peer_id)

print(derive_renewal_secret(
    b"lease secretxxxxxxxxxxxxxxxxxxxx",
    b"storage indexxxxxxxx",
    b"tub idxxxxxxxxxxxxxx",
))

I'm not really sure where to put it, though, or what kind of quality control to try to apply to it ... I suppose I could try to observe a few derived renewal secrets from the actual client software and then capture them as test vectors ... But that code isn't exactly easy to invoke.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that is what I was thinking. You could just put it after the prose section.

I was able to validate that implementation by looking at existing code, other than the logic that calculates peer_id. But... that logic doesn't match the prose spec anyway, since it's supposed to be based on x509 certificate, not tub ID? So if you fix that line I think it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

The tubid is the sha1 hash of the x509 certificate. So I think it works out correctly the way I implemented it above. It's true that "tubid" is a Foolscap concept and the code that computes it is in Foolscap.

I didn't feel like grabbing cryptography and making the certificate an input to this function. The tubid is currently quite convenient to find (it's in every storage fURL) and that won't immediately change even after we drop Foolscap (because Tahoe will have to be capable of accepting fURLs for a long, long time).

So I'd like to leave this as-is is for now but I would be happy to see it revisited in the near future to scrub all traces of Foolscap from the spec.

docs/proposed/http-storage-node-protocol.rst Outdated Show resolved Hide resolved
docs/proposed/http-storage-node-protocol.rst Show resolved Hide resolved
Copy link
Collaborator

@itamarst itamarst left a comment

Choose a reason for hiding this comment

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

Please add the code, tweaked as per comment (I can't bring myself to mentally parse the prose spec, honestly, but I'm rather sleep deprived).

@exarkun
Copy link
Member Author

exarkun commented Sep 7, 2021

Please add the code, tweaked as per comment (I can't bring myself to mentally parse the prose spec, honestly, but I'm rather sleep deprived).

I think that's fair. I'd love to use a better tool for this but I'm not really familiar with any. The convention in Tahoe seems to be to draw a picture and I'm not really up for that either.

I added the reference implementation along with some test vectors showing it agrees with the current version of Tahoe.

Copy link
Collaborator

@itamarst itamarst left a comment

Choose a reason for hiding this comment

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

Thank you.

@exarkun
Copy link
Member Author

exarkun commented Sep 7, 2021

CI isn't done but it seems to be because GitHub Actions wedged itself. I'm not gonna bother waiting for it to sort itself out.

@exarkun exarkun merged commit 5d75706 into tahoe-lafs:master Sep 7, 2021
@exarkun exarkun deleted the 3774.lease-renewal-secret-construction-docs branch September 7, 2021 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants