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.

Sign up for GitHub

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-5679] Generate and export operation #5075

Open

lvydra wants to merge 1 commit into wildfly:main from lvydra:WFCORE-5679

Contributor

lvydra commented Apr 25, 2022

https://issues.redhat.com/browse/WFCORE-5679

github-actions bot added the deps-ok label

yersan requested a review from fjuma

April 25, 2022 13:42

Contributor

fjuma commented Apr 25, 2022

@darranl Just wanted to check with you since you had created WFCORE-5679 - this issue is about introducing new runtime operations for the credential-store resource. Although it doesn't affect subsystem configuration itself, it does affect documentation. Should we create an RFE to track this?

Contributor

darranl commented Apr 27, 2022

I think lets get all the pieces together for this first and reassess if we want an RFE. IMO this probably could be an enhancement rather than a full feature.

We should double check we have sufficient tests and we will also need a PR to WildFly for the community documentation.

fjuma requested changes

View reviewed changes

...n/src/main/java/org/wildfly/extension/elytron/AbstractCredentialStoreResourceDefinition.java Outdated

                           .setMinSize(1)
                           .build();
+                  static final SimpleAttributeDefinition SIZE = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.SIZE, ModelType.STRING, true)

Contributor

fjuma Apr 27, 2022

This should be ModelType.INT and then setMinSize should be removed.

Contributor

fjuma Apr 27, 2022

Since all of these attributes are runtime only, I think you can use setStorageRuntime.

...n/src/main/java/org/wildfly/extension/elytron/AbstractCredentialStoreResourceDefinition.java Outdated

                   static final SimpleAttributeDefinition KEY = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.KEY, ModelType.STRING, false)
                           .setMinSize(1)
                           .build();
+                  static final SimpleAttributeDefinition PUBLIC_KEY_STRING = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.PUBLIC_KEY_STRING, ModelType.STRING, true)

Contributor

fjuma Apr 27, 2022

This will be a public key provided in PEM format, right? If so, I think we could just use public-key instead of public-key-string. I think this is the approach taken elsewhere in the Elytron subsystem.

...n/src/main/java/org/wildfly/extension/elytron/AbstractCredentialStoreResourceDefinition.java Outdated

+                          .setMinSize(1)
+                          .build();
+                  static final SimpleAttributeDefinition PRIVATE_KEY_STRING = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.PRIVATE_KEY_STRING, ModelType.STRING, true)

Contributor

fjuma Apr 27, 2022

Same here.

...n/src/main/java/org/wildfly/extension/elytron/AbstractCredentialStoreResourceDefinition.java Outdated

+                          if (algorithmModel.isDefined()) {
+                              algorithm = algorithmModel.asString();
+                          } else {
+                              algorithm = RSA_ALGORITHM;

Contributor

fjuma Apr 27, 2022

The default value can be defined above using the SimpleAttributeDefinitionBuilder instead of doing it here.

...n/src/main/java/org/wildfly/extension/elytron/AbstractCredentialStoreResourceDefinition.java Outdated

+                          if (sizeModel.isDefined()) {
+                              size = sizeModel.asInt();
+                          } else {
+                              size = 0;

Contributor

fjuma Apr 27, 2022

Same here.

...n/src/main/java/org/wildfly/extension/elytron/AbstractCredentialStoreResourceDefinition.java Outdated

+                          .setMinSize(1)
+                          .build();
+                  static final SimpleAttributeDefinition PUBLIC_KEY_LOCATION = new SimpleAttributeDefinitionBuilder(ElytronDescriptionConstants.PUBLIC_KEY_LOCATION, ModelType.STRING, true)

Contributor

fjuma Apr 27, 2022

Since either PUBLIC_KEY or PUBLIC_KEY_LOCATION should be specified, we should use setAlternatives to specify this. For some examples, there are other places in the Elytron subsystem where setAlternatives is used.

Contributor

fjuma Apr 27, 2022

Have you checked to verify that an error occurs if neither are specified?

Contributor Author

lvydra May 4, 2022

Since either PUBLIC_KEY or PUBLIC_KEY_LOCATION should be specified, we should use setAlternatives to specify this. For some examples, there are other places in the Elytron subsystem where setAlternatives is used.

I have added setAlternatives, however, it's not working here how it is supposed to. I'm not sure if I'm not missing something.

...n/src/main/java/org/wildfly/extension/elytron/AbstractCredentialStoreResourceDefinition.java Outdated

+                          ModelNode privateKeyLocation = PRIVATE_KEY_LOCATION.resolveModelAttribute(context, operation);
+                          ModelNode publicKeyLocation = PUBLIC_KEY_LOCATION.resolveModelAttribute(context, operation);
+                          String passphrase = PASSPHRASE.resolveModelAttribute(context, operation).asString();

Contributor

fjuma Apr 27, 2022

Looks like the PASSPHRASE is optional so it could be null here. Should it be required?

Contributor

fjuma Apr 27, 2022

In general, would be good to double check which attributes are required for each operation.

Contributor Author

lvydra May 4, 2022

Looks like the PASSPHRASE is optional so it could be null here. Should it be required?

Passphrase can be null if none was used to encrypt the key, while it is not recommended. I have added a test case to check it.

elytron/src/main/java/org/wildfly/extension/elytron/KeyPairUtil.java Outdated


		import static org.wildfly.extension.elytron._private.ElytronSubsystemMessages.ROOT_LOGGER;

		public class KeyPairUtil {

Contributor

fjuma Apr 27, 2022

Does this need to be public?

elytron/src/main/java/org/wildfly/extension/elytron/KeyPairUtil.java Outdated

+                  public static KeyPair parseKeyPair(String privateKeyContent, String publicKeyContent, ElytronFilePasswordProvider passwordProvider) {
+                      KeyPair keyPair;
+                      try {
+                          keyPair = Pem.parsePemOpenSSHContent(CodePointIterator.ofString(privateKeyContent), passwordProvider).next().tryCast(KeyPair.class);

Contributor

fjuma Apr 27, 2022

We might want to consider having a different parameter for specifying OpenSSH format so we don't try to guess the format. As an example, take a look at how this was done here:

https://github.com/wildfly-security/wildfly-elytron/blob/1.x/auth/client/src/main/resources/schema/elytron-client-1_7.xsd

Contributor

fjuma commented Apr 27, 2022

@lvydra Thanks for working on this! I've added some comments. Feel free to let me know if you have any questions.

Contributor Author

lvydra commented May 4, 2022

Hi @fjuma Thanks for the review, I have updated PR and added some comments.

wildfly-ci commented May 4, 2022

Core - Full Integration Build 11408 outcome was FAILURE using a merge of 2028ab3
Summary: Tests failed: 1 (1 new), passed: 3989, ignored: 41 Build time: 01:54:07

Failed tests

org.jboss.as.test.integration.messaging.jms.external.prefix.ExternalJMSDestinationDefinitionMessagingDeploymentTestCase.testSendMessage: java.io.IOException: java.util.concurrent.ExecutionException: java.io.IOException: HTTP Status 500 Response: <html><head><title>ERROR</title><style>
body {
    font-family: "Lucida Grande", "Lucida Sans Unicode", "Trebuchet MS", Helvetica, Arial, Verdana, sans-serif;
    margin: 5px;
}

.header {
    background-image: linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    background-image: -o-linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    background-image: -moz-linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    background-image: -webkit-linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    background-image: -ms-linear-gradient(bottom, rgb(153,151,153) 8%, rgb(199,199,199) 54%);
    
    background-image: -webkit-gradient(
        linear,
        left bottom,
        left top,
        color-stop(0.08, rgb(153,151,153)),
        color-stop(0.54, rgb(199,199,199))
    );
    color: black;
    padding: 2px;
    font-weight: normal;
    border: solid 1px;
    font-size: 170%;
    text-align: left;
    vertical-align: middle; 
    height: 32px; 
    margin-bottom: 10px;
}
.error-div {
    display: inline-block;
    width: 32px;
    height: 32px;
    background: url('data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAABmJLR0QAAAAAAAD5Q7t/AAAACXBIWXMAAABIAAAASABGyWs+AAAACXZwQWcAAAAgAAAAIACH+pydAAAGGElEQVRYw8WXW2wcVxnHf+fM7NXrdXbdtZ3ipGqCEzvQKmlFoUggwkvvGBoaqVVV5Y0nkCoQPPCAhMQbQlzUh/IWHoh6QUVKZVLxUCGogKZRW9uJkyg0iVzZ2dhre9c7u7M7c87hYWc2M7t2bBASI306O/85+v7/7zLn24H/8yV2u/EcFNvwVNK2X0DKw1qpYaX1gCWlY0m5arRe8JQ6I2DmaVj/nwk4CxO2Zf1aw9dKo6N67+eOZHN7x0gViiSHBmlXa7hrazjLt1i+eKmxUi5bwpg/e8a8PA3X/msBr0MyJ+WvgFOffehY8v7HH5OW78HqGjgOtNrge2AnIJWAgQEYLqBsm3/NvKM/mZ1rYczpRa2/9x3w/iMBM1BCiHfuGRmZevClF9NJ5cP1m9B0+/aa3t/ZDNw3TtuSfPy737trK6sXtTGPPwOruxLwNhSEEHMHpqZGDz93wubKFdio3ZXUGNOHs2cIDh/g6pt/9G8uXLnlGPP5k1Dt5bN60m6lhXj34OTkxKET37SZm4d6oz/KgFRH7yOrBozbwlTWGf7ql6RZrWTcytrxaTj9Ro9OGb3JS/mz0nDxgYnnvmWbuUsdJ6FTY2JmImShqZA83N90UbOXOXDiG4nCcPFoTsqfbluCP8DenJQLx1/+7pBcXILaZl9qo2svFmalb48xkM9hxkf5+29+u9HSenIayn0ZyEr5i0NHpnKy7aGrtW6UehsLozRBRlRQEh3BwzLpWh3h++yfPJRLSvnzvhK8DkPamOl7n3nCUjcXtyXuK0X0WViaAFc9QtSNJe594uu2MubZGcjHBGTgqeFiQQml0K12nDgSpd4iyi4uRMci6Q8xJSWq7SGEZCg/qD14LCYgIcTzoxMHc6qygYk40aETKTFSdh2aANOWhbEsjJSYCK6C/V0LcH+9xvCB/TlbiBdCAXbQMJPZ/fvQjUbXEULAa6/1deyuh0dwNU+e7DSzEOhmi8xn9iI+ujgVE6ChlBwpYcqVjuIgA3IXBMYYtNbd30IIpJQI0ZEa+gPQnk9ieA8GSrESaGNyiWIBrVQntUHqdrq01iiluo0WilBK4fs+WutOr4S9YAyJoRzamMGYAClEve046ETyTr13EKCU6kZ+N4GNCxfwKpVOryQSuOUVhBCbsRIIuN1cWRkaSCfRrdaOTncijgltNPDn5xHZLIn7x3GNh4CVmADgcv3a9YmBY0cxm06nAQH31KlOao3BL5fxbtzAOE73hAt7JXb6RU7PGO44WK5Do1ZHw6WYAN+YM+X5S8dLXziWizVNvY5aWcFfWuq8Idsctb1Yr8DwmRwrUr1yfVMbcyYmwIWZ2mbdai9+gvfBAsaAbrUwrrtllLuJPibIGGQ2hUlIHLdlA+diTXgSqkKIt5b/OevL+8bwqlWU63anW3jyhaa4c9T2Hs0hrsL5EOCJQ/sov7/gS8Mb07AZEwDgav2D5aVy3eSziEL+Dllk0PSRcmcMqx6BUeGykINsksrttboPP4w2aVfACVhGiFevvft+I/XwJCad3DF6tc2MiD4zmRSZhyZY/OusI4V4JTqKYwIA6lr/2Gm6s5/+7UMv/egDmEyqL7VbRR/LViR6sikGvniYpffm2m6r/VFd65/0vqZ9R/tbsMeG+WKpMDL+lWMJ58Jl/NVqvOG26PAoDmCXhhg4epDl9+a9zUp1uQkPbvWfcMvZchbuEXAuk0kf2X/84YxpuDQv3kA5zW1fw9CswSyZyXFEJsmnf/m46bntuTY8+SxUtuLadri9CokxeEUK8WJhtJgaeeSIxG3RurWGv+Gg3RbaU5CwkOkU1lCWxFgBmbS4ff6qX7297rXh9Gn4/llobMcjtsFywACQ+zZMTsOP8vBIOpNS+bFiJj2cxx7MYg+k8ZwmrWqD9lqNjVvrTc9tWRtw/k345dtwFagDTmTdUUA2EDAYXffB2JPw6FH4ch7GUpCzIemD50J9A1Y+hH/8Cc4vdTq9Tud9D9dNOt+MajclyNL53xZmIhusmcBSQJLOd4UJnHqACzTppDy0kHizl/yuPRB5nggIM0A6IE4GuA34EQFtoBUQuwGm7kbwb+eaEEXmuV5dAAAAJXRFWHRjcmVhdGUtZGF0ZQAyMDA5LTExLTEwVDE5OjM4OjI0LTA3OjAwdDKp4gAAACV0RVh0ZGF0ZTpjcmVhdGUAMjAxMC0wMi0yMFQyMzoyNjoyNC0wNzowMC7DUNYAAAAldEVYdGRhdGU6bW9kaWZ5ADIwMTAtMDEtMTFUMDg6NTc6MzUtMDc6MDCruapPAAAAMnRFWHRMaWNlbnNlAGh0dHA6Ly9lbi53aWtpcGVkaWEub3JnL3dpa2kvUHVibGljX2RvbWFpbj/96s8AAAAldEVYdG1vZGlmeS1kYXRlADIwMDktMTEtMTBUMTk6Mzg6MjQtMDc6MDArg9/WAAAAGXRFWHRTb3VyY2UAVGFuZ28gSWNvbiBMaWJyYXJ5VM/tggAAADp0RVh0U291cmNlX1VSTABodHRwOi8vdGFuZ28uZnJlZWRlc2t0b3Aub3JnL1RhbmdvX0ljb25fTGlicmFyebzIrdYAAAAASUVORK5CYII=') left center no-repeat;
}
.error-text-div {
    display: inline-block;
    vertical-align: top;
    height: 32px;
}
.label {
    font-weight:bold;
    display: inline-block;
}
.value {
    display: inline-block;
    margin-left: 5px;
}
pre {
    font-size: 110%;

Collaborator

yersan commented May 27, 2022

hi @lvydra, it looks like errors are related to your changes. Could you take a look?

yersan added Feature missing-reqs labels

Contributor Author

lvydra commented May 27, 2022

Hi @yersan, thanks, I will look at it.

fjuma reviewed

View reviewed changes

elytron/src/test/java/org/wildfly/extension/elytron/CredentialStoreTestCase.java Outdated

@@ @@ -221,13 +222,14 @@ public void testExportPublicKey() { @@
                       Assert.assertTrue(publicKey.contains("ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQ"));
                   }
+                  @Ignore

Contributor

fjuma Jun 3, 2022

@lvydra Feel free to let us know when this is ready for another review. Since this test is currently being ignored, it looks like you're working on fixing the test failures, right?

Contributor Author

lvydra Jun 6, 2022

Hi @fjuma, yes, I'm working on fixing test failures. Ignored tests are written to test setAlternatives behavior, which is currently not working as it's supposed to, so I'm looking for a way how to fix that.

lvydra force-pushed the WFCORE-5679 branch 2 times, most recently from fbc1ca2 to 90e2dcf Compare

June 21, 2022 13:44

Contributor Author

lvydra commented Jun 23, 2022

Hi @fjuma, I think that the PR should be ready for another round of review :-) All requests should be addressed, the only persisting problem is setAlternatives validation, which I haven't been able to get working. I have added a custom validation method using alternatives attribute, however, I don't think it's a suitable solution.

fjuma reviewed

View reviewed changes

elytron/src/main/java/org/wildfly/extension/elytron/CredentialStoreResourceDefinition.java Outdated

+                              throw ROOT_LOGGER.credentialAlreadyExists(alias, KeyPairCredential.class.getName());
+                          }
+                          ModelNode privateKeyString = PRIVATE_KEY.resolveModelAttribute(context, operation);

Contributor

fjuma Jun 27, 2022

For optional attributes like this, you could use String privateKey = PRIVATE_KEY.resolveModelAttribute(context, operation).asStringOrNull() so you won't need the isDefined() checks and asString() calls below.

elytron/src/test/java/org/wildfly/extension/elytron/CredentialStoreTestCase.java

+                  }
+                  @Test
+                  public void testExportPublicKey() {

Contributor

fjuma Jun 27, 2022

Would also be good to add a test for the case where the alias doesn't exist.

Contributor

fjuma commented Jun 27, 2022

Thanks for the updates, @lvydra! I've added some comments.

Contributor

fjuma commented Jun 27, 2022

What is the issue you're running into with setAlternatives? Is it that it's not actually taking an effect? I wonder if it's because these attributes are only being used for runtime operations.

Contributor Author

lvydra commented Jun 28, 2022 •

edited

Hi @fjuma, thanks for the review, I have updated PR.
Yes exactly, it doesn't take any effect.

github-actions bot commented Aug 13, 2022

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

github-actions bot added the Stale label

Contributor

fjuma commented Aug 15, 2022

This PR is still active.

@lvydra Looks like there's a conflict that now needs to be resolved.

wildfly-ci commented Aug 15, 2022

Hello, lvydra. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

github-actions bot removed the Stale label

lvydra force-pushed the WFCORE-5679 branch from b045cf9 to 7c7f811 Compare

August 16, 2022 07:15

Contributor Author

lvydra commented Aug 16, 2022

Hi @fjuma, thanks, resolved.

Collaborator

yersan commented Sep 23, 2022

@fjuma / @darranl Is there any decision about handling this as an enhancement vs Feature Request?
Since there is a user controllable action here, probably it is more inclined to a Feature Request, however, it seems it doesn't change the subsystem configuration, so here are my doubts.

Contributor

fjuma commented Oct 3, 2022

@lvydra Thanks for the updates! Sorry for the delayed response, just catching up after being away for a couple weeks.

Just to check, have you already created a PR against WildFly with the corresponding community documentation?

Contributor Author

lvydra commented Oct 4, 2022

Hi @fjuma, not yet, I will prepare a documentation update and open PR.

Contributor Author

lvydra commented Oct 6, 2022

Hi @fjuma, I have opened documentation PR: wildfly/wildfly#16139

fjuma mentioned this pull request

[WFLY-17073] KeyPairCredentials management operations documentation wildfly/wildfly#16139

Open

github-actions bot commented Nov 21, 2022

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

github-actions bot added the Stale label

Contributor

fjuma commented Nov 21, 2022

This PR is still active.

github-actions bot removed the Stale label

github-actions bot commented Jan 7, 2023

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

github-actions bot added the Stale label

Contributor

fjuma commented Jan 9, 2023

This PR is still active.

github-actions bot removed the Stale label

github-actions bot commented Feb 25, 2023

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

github-actions bot added the Stale label

github-actions bot commented May 27, 2023

There has been no activity on this PR for 90 days and it has been closed automatically.

github-actions bot closed this

lvydra reopened this


          [WFCORE-5679] Generate and export operation

e692c1b

lvydra force-pushed the WFCORE-5679 branch from 7c7f811 to e692c1b Compare

June 26, 2023 17:18

github-actions bot removed the Stale label

github-actions bot commented Aug 12, 2023

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

github-actions bot added the Stale label

Contributor

fjuma commented Aug 14, 2023

This PR is still active

github-actions bot removed the Stale label

github-actions bot commented Sep 29, 2023

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

github-actions bot added the Stale label

github-actions bot commented Dec 28, 2023

There has been no activity on this PR for 90 days and it has been closed automatically.

github-actions bot closed this

lvydra reopened this

github-actions bot removed the Stale label

github-actions bot commented Mar 17, 2024

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

github-actions bot added the Stale label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment