Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add SafeURL helper method to GitRemoteConfig #2549

Merged
merged 1 commit into from
Oct 24, 2019

Conversation

hiddeco
Copy link
Member

@hiddeco hiddeco commented Oct 24, 2019

During the development of the (secure) Git HTTPS credential feature,
I did not take the response of the GitRepoConfig API method into
account. As a direct result, the fluxctl sync command still exposes
the full Git URL in the logs.

This commit embeds the git.Remote in the GitRemoteConfig structure,
so that we can re-use the SafeURL helper of this structure in the
fluxctl code. To ensure the JSON representation stays the same, a
JSON struct tag has been added to the git.Remote URL field.

Fixes #2548

@squaremo
Copy link
Member

Thought: would implementing Remote.String() (and perhaps making URL unexported) help with some of the messing around with SafeURL()?

@hiddeco
Copy link
Member Author

hiddeco commented Oct 24, 2019

@squaremo I wasn't sure what modifications I could make to the structure without it effecting e.g. Weave Cloud or flux-web. If you think it is safe to do so, I am happy to adjust the commit.

@squaremo
Copy link
Member

I wasn't sure what modifications I could make to the structure without it effecting e.g. Weave Cloud or flux-web

Hmm yeah true, it might affect serialisation. OK, we can leave that for now.

pkg/api/v6/api.go Outdated Show resolved Hide resolved
@hiddeco hiddeco force-pushed the fluxctl/no-https-creds-leak branch from 0060275 to 9925769 Compare October 24, 2019 11:56
Copy link
Member

@squaremo squaremo left a comment

Choose a reason for hiding this comment

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

LGTM

During the development of the (secure) Git HTTPS credential feature,
I did not take the response of the `GitRepoConfig` API method into
account. As a direct result, the `fluxctl sync` command still exposes
the full Git URL in the logs.

This commit embeds the `git.Remote` in the `GitRemoteConfig` structure,
so that we can re-use the `SafeURL` helper of this structure in the
`fluxctl` code. To ensure the JSON representation stays the same, a
JSON struct tag has been added to the `git.Remote` URL field.
@hiddeco hiddeco force-pushed the fluxctl/no-https-creds-leak branch from 9925769 to f4a94c0 Compare October 24, 2019 13:53
@hiddeco hiddeco merged commit 14ea6c7 into master Oct 24, 2019
@hiddeco hiddeco deleted the fluxctl/no-https-creds-leak branch October 24, 2019 14:10
@2opremio 2opremio added this to the 1.16.0 milestone Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fluxctl exposes GIT_AUTHKEY in console output
3 participants