Skip to content

Conversation

@jakedoublev
Copy link
Contributor

@jakedoublev jakedoublev commented Apr 12, 2024

Closes #105
Closes #75

Will handle #129 separately

@jakedoublev jakedoublev requested a review from a team as a code owner April 12, 2024 20:35
@jakedoublev jakedoublev marked this pull request as draft April 12, 2024 20:38
@jakedoublev jakedoublev marked this pull request as ready for review April 22, 2024 16:39
@b-long
Copy link

b-long commented Apr 24, 2024

Hey @jakedoublev , cool feature! I've been playing with this over the last few days and noticed that it just started working (at least with the configuration I'm using).

On my machine, I'm standing up the platform according to the platform "Run" section, here: https://github.com/opentdf/platform#run .

Once the platform is running, I've been doing the following:

# Checkout this branch
git checkout feat/tdf-demo

# Build `otdfctl`
make

# Get Help
cp otdfctl.yaml target/ && pushd target && { ./--darwin-arm64 --help ; } && popd

# Authenticate
cp otdfctl.yaml target/ && pushd target && { ./--darwin-arm64 auth client-credentials --client-id opentdf --client-secret secret --log-level DEBUG ; } && popd

# Encrypt
#
# NOTE: This will create a file 'sensitive.txt.tdf'
cp otdfctl.yaml target/ && pushd target && { echo "Some string" | ./--darwin-arm64 encrypt || echo "Failed to encrypt" ; } && popd

# Decrypt
cp otdfctl.yaml target/ && pushd target && { ./--darwin-arm64 decrypt sensitive.txt.tdf -o decrypted.txt || echo "Failed to decrypt" ; } && popd

Previously I'd get an error about DPoP:

"time":"2024-04-23T11:58:54.970023-04:00","level":"DEBUG","msg":"LoadConfig: file and key not provided, using default file","config file":""}
{"time":"2024-04-23T11:58:55.216295-04:00","level":"DEBUG","msg":"getting new access token"}
{"time":"2024-04-23T11:58:55.216339-04:00","level":"DEBUG","msg":"Building DPoP Proof"}
 ERROR    Failed to decrypt file: reader.doPayloadKeyUnwrap failed:  splitKey.rewrap failed:error making request to kas: error making rewrap request: rpc error: code = Unauthenticated desc = unauthenticated

As of this morning, I'm not getting this error message! 🎉

I'm guessing one of the recent platform commits (e.g. maybe this DPoP change?) has unlocked this for me. Of course, that's a total guess, and doesn't really matter 😃

That said, there does seem to be a small bug in the decrypt command. I am specifying -o decrypted.txt, but it looks like the output is coming to STDOUT. Have you seen that behavior?

@jrschumacher
Copy link
Member

@b-long Yes the DPoP change makes it optional since we've been having some issues supporting DPoP across IdPs given that it is still a proposed standard.

Thanks for trying this out and giving feedback!

@b-long
Copy link

b-long commented Apr 24, 2024

@jakedoublev @jrschumacher Here comes a "stupid question" from me, so apologies in advance as I'm just learning golang.

Is this a reasonable fix to -o? #128

If not, can you provide guidance about how it can be improved?

@jakedoublev
Copy link
Contributor Author

@b-long Thanks for your engagement, and glad you're excited about this feature. :) I think your proposed fix for -o should be sufficient, but I actually wonder if we may want to skip this flag altogether and make it a convention to write stdout to a file instead (i.e. ./otdfctl decrypt sensitive.txt.tdf > output.txt).

Flag -o pros and cons:
🟩 : decrypted content and decrypted content alone are written to the output file
🟨 : if writing stdout to a file, the only logs should be debug, and running in debug mode will have known side effects on stdout
🟥 : if encrypt reads from stdin, it seems like decrypt should write to stdout
🟥 : reserves the -o flag namespace permanently

Would the flow ./otdfctl decrypt sensitive.txt.tdf > output.txt meet your needs, in which case I could remove the flag altogether and update the manual/docs?

@b-long
Copy link

b-long commented Apr 24, 2024

Personally, I like having -o as an option. Especially , given this pro:

🟩 : decrypted content and decrypted content alone are written to the output file

That said, I also think it's nice to have an option to write to STDOUT. I like both 🙂

RE: Your other points:

🟨 : if writing stdout to a file, the only logs should be debug, and running in debug mode will have known side effects on stdout

I'm not too worried about the logging behavior. Except, if you remove the -o feature. If -o (the feature) is removed then I worry about the risk of log content being written and my decrypted file malformed (e.g. anything that isn't just text; .png, .pdf, .mov files)

🟥 : if encrypt reads from stdin, it seems like decrypt should write to stdout

I agree, piping is a useful feature 👍

🟥 : reserves the -o flag namespace permanently

Happy to use any flag, but I think the feature (write to a file, explicitly) is useful. 👍

Would the flow ./otdfctl decrypt sensitive.txt.tdf > output.txt meet your needs

I think if we made this the only option , we'd be losing something that seems valuable 🤷

@b-long
Copy link

b-long commented Apr 24, 2024

Last thing, just to clarify. The -o flag isn't something I introduced. It was there (I found it via --help), but it was broken.

My PR was just meant to fix it, not introduce it 🙂

@b-long b-long self-assigned this Apr 24, 2024
@b-long b-long removed their assignment Apr 24, 2024
@b-long b-long requested review from b-long and removed request for a team April 24, 2024 15:46
b-long
b-long previously approved these changes Apr 24, 2024
b-long
b-long previously approved these changes Apr 24, 2024
@jakedoublev jakedoublev merged commit 3c50c2d into main Apr 25, 2024
@jakedoublev jakedoublev deleted the feat/tdf-demo branch April 25, 2024 12:53
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.

Demo: CLI encrypt/decrypt TDF flow powered by otdfctl CLI should be able to authenticate with new casbin-driven auth middleware

4 participants