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

fix file and cmd secret source inputs #6845

Merged
merged 1 commit into from Mar 12, 2024

Conversation

kpenfound
Copy link
Contributor

Reported here: #6829 (comment)

Based on my testing, the following did not work:

dagger call foo --token env:TOKEN
echo $TOKEN > token.txt
dagger call foo --token file:./token.txt

Breaking out the file parsing code, the token.txt would be parsed with a newline character at the end.

This adds TrimSpace() to file and cmd secret inputs. TrimSpace() trims leading and trailing unicode whitespace characters. I can't think of any cases where this would have unintended consequences for secrets.

Signed-off-by: Kyle Penfound <kyle@dagger.io>
@kpenfound kpenfound requested a review from sipsma March 6, 2024 19:42
Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

Hm yeah I agree this is likely what users would want 99.99% of the time and fairly annoying to require them to deal with themselves in userspace (i.e. get secret plaintext, strip newlines, create new modified secret).

I feel like there must be some obscure corner case where this isn't desired, but I can't think of any concretely either. And push comes to shove if we need to support the 0.01% of cases we can always add some kludge like
--token=file-raw:/some/token/file/where/trailing/newlines/matter
or similar.

cc @vito @shykes @jedevc in case you know of any concrete cases where this wouldn't be desired

Copy link
Contributor

@vito vito left a comment

Choose a reason for hiding this comment

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

Yeah seems worth the trade-off to me.

I do remember being bitten in the past by something that wasn't happy with a TLS certificate without a trailing linebreak. But I'd rather say whatever tool that was is wrong. Unless that tool is openssl or something. 😅

@gerhard gerhard added this to the v0.10.2 milestone Mar 12, 2024
@gerhard gerhard merged commit 5fbe1b2 into dagger:main Mar 12, 2024
43 checks 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.

None yet

4 participants