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

feat: add endpoints for putting and getting tails files by hash #53

Merged
merged 5 commits into from
Aug 10, 2023

Conversation

dbluhm
Copy link

@dbluhm dbluhm commented Aug 1, 2023

This PR adds endpoints for putting and getting tails files by hash. This change is to support using this project with ledgers other than just Indy where the revocation registry ID might not be URL friendly and so the tails server doesn't have to know how to resolve objects from any arbitrary ledger.

The PUT /hash/{tails-hash} endpoint will validate the hash against the uploaded file and will also ensure the tails file "looks" like a tails file. Details of the checks can be found in the comments on the code.

Signed-off-by: Char Howland <char@indicio.tech>
Signed-off-by: Char Howland <char@indicio.tech>
Signed-off-by: Char Howland <char@indicio.tech>
Signed-off-by: Char Howland <char@indicio.tech>
Signed-off-by: Char Howland <char@indicio.tech>
# Tails file must start with "00 02"
tmp_file.seek(0)
if tmp_file.read(2) != b'\x00\x02':
raise web.HTTPBadRequest(text='Tails file must start with "00 02".')
Copy link
Author

Choose a reason for hiding this comment

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

Perhaps it would be better to not be so specific about what went wrong?

@@ -46,7 +46,7 @@ exportEnvironment() {
export GENESIS_URL=${GENESIS_URL:-http://$DOCKERHOST:9000/genesis}
export STORAGE_PATH=${STORAGE_PATH:-/tmp/tails-files}
export LOG_LEVEL=${LOG_LEVEL:-INFO}
export TAILS_SERVER_URL=${TAILS_SERVER_URL:-http://$DOCKERHOST:6543}
export TAILS_SERVER_URL=${TAILS_SERVER_URL:-http://host.docker.internal:6543}
Copy link
Author

Choose a reason for hiding this comment

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

We can probably not include this change.

Copy link
Member

@WadeBarnes WadeBarnes Aug 11, 2023

Choose a reason for hiding this comment

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

This change may not work on Linux. I'm not sure if the Linux version of Docker has caught up with the Windows and Mac version that use host.docker.internal automatically. The script was originally setup to autodetect the docker host URL or IP based on the platform. Refer to the script at the top of the file.

# getDockerHost; for details refer to https://github.com/bcgov/DITP-DevOps/tree/main/code/snippets#getdockerhost
. /dev/stdin <<<"$(cat <(curl -s --raw https://raw.githubusercontent.com/bcgov/DITP-DevOps/main/code/snippets/getDockerHost))"
export DOCKERHOST=$(getDockerHost)

If the Linux version of Docker has caught up and is host.docker.internal aware, you can ignore this comment.

Copy link
Member

@WadeBarnes WadeBarnes Aug 11, 2023

Choose a reason for hiding this comment

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

Looking through the docker release notes, it appears using host.docker.internal still requires some manual steps to work on Linux; it's not automatic like on Mac and Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve reverted this change in the next PR as I hit this problem in testing.

@swcurran
Copy link
Contributor

Happy to merge as is. Anything bad likely to happen with the docker.host change? I’m not good at PRs to PRs, so we can address after.

@swcurran swcurran merged commit fa0fc24 into bcgov:main Aug 10, 2023
1 check passed
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.

4 participants