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

Jump to bottom

[WFCORE-4484] Support SSH Authentication for Git persistence #249

Merged
merged 1 commit into from Oct 13, 2020
Merged

[WFCORE-4484] Support SSH Authentication for Git persistence #249

merged 1 commit into from Oct 13, 2020

Conversation

ashley-abdelsayed98
Copy link
Contributor

fjuma
fjuma reviewed Oct 8, 2019
View reviewed changes
elytron/WFCORE-4484-ssh-authentication-git-persistence.adoc Outdated
=== Hard Requirements

==== JGit/SSHD
* It should be possible to connect to a remote git repo and authenticate with SSH to manage the Git configuration file history.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor comment, it would be good to explicitly mention Elytron here, e.g., "It should be possible to connect to a remote git repo using Elytron..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

elytron/WFCORE-4484-ssh-authentication-git-persistence.adoc Outdated

The option will also be added to specify the keys to be used for authentication as KeyPairCredentials. This will mean
supporting specifying KeyPairCredentials in an Elytron configuration file as a credential or as a reference to an entry in
a CredentialStore. This will require modifying the KeyPairCredential to accept the OpenSSH public and private key formats.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add a link to the specification of these formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

elytron/WFCORE-4484-ssh-authentication-git-persistence.adoc Outdated
keys in the new OpenSSH format
** There will be the option to reference a KeyPairCredential stored in a CredentialStore
** It will be possible to specify the location and identity of a private key (eg. ~/.ssh/id_rsa). The user will also
be able to specify the passphrase used to encode the key.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to allow the passphrase to be specified using a credential-reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

elytron/WFCORE-4484-ssh-authentication-git-persistence.adoc Outdated
It will also be possible for users to specify the keys to be used for authentication as KeyPairCredentials. This will mean
supporting specifying KeyPairCredentials in an Elytron configuration file as a credential or as a reference to an entry in
a CredentialStore. This will require modifying the KeyPairCredential to accept the OpenSSH public and private key formats.
The KeyPairCredential type will also support the RSA, DSA, ECDSA, and EDDSA algorithms. The user should then be able to
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to mention here which of these algorithms are already supported and which ones will need to be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

elytron/WFCORE-4484-ssh-authentication-git-persistence.adoc Outdated
* Tests will be added to the WildFly-Core testsuite to test SSH authentication when connecting to a Git repository
** Tests will be added to test successful authentication when specifying the location, name, and passphrase to the file
containing a private key
** Tests will be added to test successful authentication using a KeyPairCredential with all supported algorithm types
Copy link
Contributor

Choose a reason for hiding this comment

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

It would also be good to consider tests for the failed authentication case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

elytron/WFCORE-4484-ssh-authentication-git-persistence.adoc Outdated
* Documentation will be added to https://github.com/wildfly/wildfly/blob/master/docs/src/main/asciidoc/_elytron/Credential_Store.adoc[Credential Store]
to describe the new options to generate and import KeyPairCredentials
* Documentation will be added to https://github.com/wildfly/wildfly/blob/master/docs/src/main/asciidoc/_elytron/Client_Authentication_with_Elytron_Client.adoc[Client Authentication with Elytron Client]
to describe the changes to the KeyPairCredential and the new option to specify a private keys location as a credential.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add a "Release Notes" section at the end here. This was recently added to the proposal template. See https://lists.jboss.org/pipermail/wildfly-dev/2019-October/007018.html for more details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

This was referenced Nov 7, 2019
Merged
Closed
mnovak1
mnovak1 reviewed Nov 28, 2019
View reviewed changes
elytron/WFCORE-4484-ssh-authentication-git-persistence.adoc
a CredentialStore. This will require modifying the `key-pair` credential to accept the OpenSSH
https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.key[private key] format. The user should then be able to
configure their WildFly standalone server to connect to a Git repo and authenticate with SSH to manage their configuration file history.

Copy link

Choose a reason for hiding this comment

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

This feature might be interesting for Wildfly/EAP Openshift images where configuration might be taken from external git repository and not stored in image or created during container start.

Such usage should be documented and quickstart could be added, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds like a good idea, I'll add it to the documentation and look at working on a quickstart. Thank you!

Copy link

Choose a reason for hiding this comment

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

What about the quickstart? Will it be part of this RFE, or in a subsequent RFE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darranl I was wondering if you think it would be better to include the quickstart as part of this RFE or hold off and create a follow up RFE?

Also, as a side note when I was looking into testing out this feature with OpenShift, I was following these steps: https://github.com/wildfly/wildfly-s2i#building-the-images. I tried following the "Building WildFly s2i builder image with a locally built WildFly server" steps to test out my local changes for this RFE but I would get the following error:

 Non-resolvable import POM: Failure to find org.wildfly.core:wildfly-core-parent:pom:11.0.0.Beta6-SNAPSHOT in https://repository.jboss.org/nexus/content/groups/public/ was cached in the local repository,

So I was wondering if you knew the steps to be able to use my local WildFly-Core changes to build the WildFly image?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a follow up RFE could be created for the quickstart. @darranl What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 any Quickstart should really be an RFE of it's own, however this RFE is only adding support for an authentication mechanism, this RFE does not add support for Git persistence so the Quickstart RFE should be against Management and not against Security.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've created https://issues.redhat.com/browse/EAP7-1521 to track the creation of a quickstart for git persistence.

@ehsavoie ehsavoie mentioned this pull request Sep 11, 2020
Merged
@bstansberry bstansberry merged commit 39b9713 into wildfly:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darranl darranl darranl left review comments

@mnovak1 mnovak1 mnovak1 left review comments

@OndrejKotek OndrejKotek OndrejKotek left review comments

@fjuma fjuma fjuma approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ashley-abdelsayed98 @darranl @mnovak1 @fjuma @OndrejKotek @bstansberry