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

feat(artifacts/gitRepo): support SSH auth #4052

Merged
merged 4 commits into from
Sep 27, 2019

Conversation

ethanfrogers
Copy link
Contributor

adds support for SSH authentication and also improves UX around what
happens if the users supplies a reference that isn't supported by the
auth type.

@ethanfrogers ethanfrogers force-pushed the git-repo-ssh-support branch 3 times, most recently from 44ea60b to c81daa0 Compare September 26, 2019 14:38
Copy link
Contributor

@ezimanyi ezimanyi left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few small comments.

authType = AuthType.HTTP;
} else if (!StringUtils.isEmpty(token)) {
authType = AuthType.TOKEN;
} else if (!StringUtils.isEmpty(sshPrivateKeyFilePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to support the case where users have an SSH key without a passphrase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly? I copied all of this logic from the App Engine logic so I assume most cases have been accounted for since it's been around longer. If cases like this come up I don't see why we couldn't just relax this condition in a patch.

new JschConfigSessionFactory() {
@Override
protected void configure(OpenSshConfig.Host hc, Session session) {
if (sshKnownHostsFilePath == null && sshTrustUnknownHosts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a use case for trusting unknown hosts even if you provide a known hosts file?

defaultJSch.addIdentity(sshPrivateKeyFilePath, sshPrivateKeyPassphrase);

if (sshKnownHostsFilePath != null && sshTrustUnknownHosts) {
log.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like this warning could be closer to where we actually ignore the value of sshTrustUnknownHosts (ie, in confgure). (Or maybe that gets called too often to log...)

add support for ssh as an alternative authentication mechanisim
improve UX around errors for authentication. if the accounts
authentication type doesn't support references of a type throw an error
before trying to download it.
add support for ssh without a passphrase
@ethanfrogers ethanfrogers merged commit b234aca into spinnaker:master Sep 27, 2019
@ethanfrogers ethanfrogers deleted the git-repo-ssh-support branch September 27, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants