Improving --signer-fulcio-token flag to accept both path and raw token string
#82
Conversation
or a raw token string
or a raw token string Signed-off-by: chaosinthecrd <[email protected]>
Signed-off-by: chaosinthecrd <[email protected]>
…tness into improving-token-flag
Signed-off-by: chaosinthecrd <[email protected]>
…tness into improving-token-flag
signer/fulcio/fulcio_test.go
Outdated
| // NOTE: this function could be refactored to accept a fileSystem or io.Reader so reading the file can be mocked, | ||
| // but unsure if this is the way we want to go for now |
There was a problem hiding this comment.
I like this idea. The less code we have in the test (i.e. composing a file path) the better. I think you could also do a relative path: ../../hack/test.token. I also don't have a strong preference on where to put test data like that.
signer/fulcio/fulcio.go
Outdated
|
|
||
| // idToken tries to parse a string as a token in JWS form. If it fails, | ||
| // it treats the string as a path and tries to open the file at that path | ||
| func idToken(s string) (string, error) { |
There was a problem hiding this comment.
https://dave.cheney.net/2019/07/09/clear-is-better-than-clever
https://go-proverbs.github.io/
The design of a function to fall back from one behavior to another can be both practical and user-friendly, but it also has potential drawbacks that need to be considered. Here are some points to consider regarding the design of the idToken function:
Pros:
-
User Convenience: The function provides flexibility by accepting either a raw token or a file path. This can be convenient for users with different forms of tokens.
-
Simplicity: The function abstracts away the details of how the token is obtained, simplifying the interface for the caller.
-
Fallback Mechanism: A fallback mechanism can be a good defensive programming practice, ensuring the function has an alternative way to proceed if the first method fails.
Cons:
-
Ambiguity: The dual-purpose nature of the function's argument can lead to ambiguity. It's not immediately clear from the function signature what the expected input is.
-
Error Handling: If the input is neither a valid token nor a valid file path, the error returned may not be informative enough to help the user understand the issue.
-
Security Concerns: Automatically treating the input as a file path if it's not a valid token could lead to security issues, such as unintentional file reads.
-
Single Responsibility Principle: The function parses a token and reads a file. This goes against the Single Responsibility Principle, which suggests that a function should do one thing only.
-
Testing Complexity: The function may be harder to test because it has multiple execution paths depending on the input.
There was a problem hiding this comment.
@naveensrinivasan - thank you so much for this comment! I was thinking exactly this when I was creating the PR. @mikhailswift @jkjell @kairoaraujo @colek42 and others, it would be good to get your perspectives on this one so we can decide a way to proceed with the PR 😄
There was a problem hiding this comment.
That’s a good comment.
I’d definitely recommend splitting up the functionality. Also from a security perspective would be more defensive to treat the suspected JWS as only ever being a JWS even if it fails to parse and have a different path for matching to a file.
Perhaps a different flag or envvar or however else it’s getting picked up.
Being explicit is better than implicit in this case I think.
There was a problem hiding this comment.
Agree with @fkautz . There's a certain nicety when things just happen for you implicitly, which is always attractive to make things convenient, but I tend to lean toward explicit options being better for use cases like this, especially due to the logic of "well this isn't a token, so let's try opening it like a file" being particularly scary.
Signed-off-by: chaosinthecrd <[email protected]>
Signed-off-by: chaosinthecrd <[email protected]>
Signed-off-by: chaosinthecrd <[email protected]>
Signed-off-by: chaosinthecrd <[email protected]>
Signed-off-by: chaosinthecrd <[email protected]>
|
One request I have is prior to merge, can this be rebased/squashed down and made to linear history? I can jump in and help with this if needed :) |
Github squashes them for us on merge and I believe that will make things linear history (one commit) 😄 |
As a result of in-toto/witness#293, I decided it might be a good idea to propose modifying the
--signer-fulcio-tokenflag to accept both a filesystem path and a raw token string. A small utility function has been added to facilitate this.For the unit test, I decided to add a "test" token file to the
hack/directory to be read by the test, instead of making the function allow anio.Readeror mock filesystem to be passed into the utility function.