Remove from URLs query parameters that might have secrets #195
Conversation
kusto/ingest/file_options.go
Outdated
| func InferFormatFromFileName(fName string) DataFormat { | ||
| return properties.DataFormatDiscovery(fName) | ||
| return properties.DataFormatDiscovery(properties.RemoveQueryParamsFromUrl(fName)) |
There was a problem hiding this comment.
How come this is relevant here?
There was a problem hiding this comment.
Every place I did this, it's a place where:
- Secrets are unnecessary for the called function to work.
- The function (perhaps transitively) can throw an error when doing something with the URL. I'm less familiar with Go's error handling, but my concern is that the exception may include the parameter.
kusto/ingest/ingest.go
Outdated
| @@ -139,7 +139,7 @@ func (i *Ingestion) FromFile(ctx context.Context, fPath string, options ...FileO | |||
|
|
|||
| // fromFile is an internal function to allow managed streaming to pass a properties object to the ingestion. | |||
| func (i *Ingestion) fromFile(ctx context.Context, fPath string, options []FileOption, props properties.All) (*Result, error) { | |||
| local, err := queued.IsLocalPath(fPath) | |||
| local, err := queued.IsLocalPath(properties.RemoveQueryParamsFromUrl(fPath)) | |||
There was a problem hiding this comment.
How come this is relevant here?
| @@ -219,7 +219,7 @@ func (i *Ingestion) Blob(ctx context.Context, from string, fileSize int64, props | |||
|
|
|||
| props.Ingestion.RetainBlobOnSuccess = !props.Source.DeleteLocalSource | |||
|
|
|||
| err = CompleteFormatFromFileName(&props, from) | |||
| err = CompleteFormatFromFileName(&props, properties.RemoveQueryParamsFromUrl(from)) | |||
There was a problem hiding this comment.
Can we do this only where we return an error or an object where the full URI is not needed?
There was a problem hiding this comment.
Maybe, I'm no Go expert, so I can look into whether we can remove the unnecessary params just when throwing the error.
But in each instance, the secrets are unnecessary to the function of the function. And perhaps from a "least privilege" standpoint, it's best not to pass secrets when not needed, notably in case a new trace/error/etc will be added in the future.
There was a problem hiding this comment.
This is not the right way to enforce this.
If you want to do something covering all possible paths, create a SecretiveUri like structure and have String() and SecureString() differ on the output.
There was a problem hiding this comment.
Not 100% sure
I get the idea, and type safety is good, but I wonder if it can even be done correctly without infecting the whole code and having breaking changes
There was a problem hiding this comment.
As discussed, I'm going to reduce the scope of this to only the explicit prints.
Afterwards, when I have time, I'll pass an object that contains both a secretive and a sanitized URL, and pass the latter to the methods that don't require the secret.
| @@ -416,6 +416,13 @@ func (i Ingestion) validate() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func RemoveQueryParamsFromUrl(url string) string { | |||
There was a problem hiding this comment.
it might be easier to use if the API was func (s string) RemoveQueryParamsFromUrl() string
calling would simply be s.RemoveQueryParamsFromUrl()
There was a problem hiding this comment.
I'm definitely not an expert in Go, but I tried that (it was my preferred approach as well), and it didn't work. IIRC the issue was that we're not allowed to add a function on the string class.
IIRC we do have a String class, and I was able to add this method to there, but I believe it would've required more extensive changes to pass that type of parameter instead of the existing string.
There was a problem hiding this comment.
Yihezkel is right, you can't monkeypatch classes on go (you can't do anything fun on go).
The capital String class is for the string type in kusto (parsing them, escaping them, etc), and should not be used for general purpose.
kusto/ingest/streaming.go
Outdated
| @@ -59,7 +59,7 @@ func (i *Streaming) FromFile(ctx context.Context, fPath string, options ...FileO | |||
| } | |||
|
|
|||
| func prepFileAndProps(fPath string, props *properties.All, options []FileOption, client ClientScope) (*os.File, error) { | |||
| local, err := queued.IsLocalPath(fPath) | |||
| local, err := queued.IsLocalPath(properties.RemoveQueryParamsFromUrl(fPath)) | |||
kusto/ingest/streaming.go
Outdated
| @@ -82,7 +82,7 @@ func prepFileAndProps(fPath string, props *properties.All, options []FileOption, | |||
| props.Source.DontCompress = true | |||
| } | |||
|
|
|||
| err = queued.CompleteFormatFromFileName(props, fPath) | |||
| err = queued.CompleteFormatFromFileName(props, properties.RemoveQueryParamsFromUrl(fPath)) | |||
| @@ -416,6 +416,13 @@ func (i Ingestion) validate() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func RemoveQueryParamsFromUrl(url string) string { | |||
| if idx := strings.Index(url, "?"); idx != -1 { | |||
There was a problem hiding this comment.
add also
strings.Index(url, ";")
Will reimplement the rest in a future PR, using agreed-upon approach.
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
=======================================
Coverage 49.59% 49.59%
=======================================
Files 53 53
Lines 5835 5835
=======================================
Hits 2894 2894
Misses 2733 2733
Partials 208 208
|
…are currently relevant to quickstart)
Security
Remove query parameters from URL when they might have secrets and might be traced (even if part of error flow)