exceptionfactory commented on code in PR #8890:
URL: https://github.com/apache/nifi/pull/8890#discussion_r1618922463
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubFlowRegistryClient.java:
##########
@@ -128,7 +128,22 @@ public class GitHubFlowRegistryClient extends
AbstractFlowRegistryClient {
.sensitive(true)
.dependsOn(AUTHENTICATION_TYPE,
GitHubAuthenticationType.APP_INSTALLATION_TOKEN.name())
.build();
-
+ static final PropertyDescriptor PRIVATE_KEY = new
PropertyDescriptor.Builder()
+ .name("Private Key")
+ .description("Private RSA key generated foo Github App to use for
Authentication")
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .required(true)
+ .sensitive(true)
+ .dependsOn(AUTHENTICATION_TYPE,
GitHubAuthenticationType.PRIVATE_KEY.name())
+ .build();
+ static final PropertyDescriptor APP_ID = new PropertyDescriptor.Builder()
+ .name("APP ID")
Review Comment:
```suggestion
.name("App ID")
```
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubFlowRegistryClient.java:
##########
@@ -128,7 +128,22 @@ public class GitHubFlowRegistryClient extends
AbstractFlowRegistryClient {
.sensitive(true)
.dependsOn(AUTHENTICATION_TYPE,
GitHubAuthenticationType.APP_INSTALLATION_TOKEN.name())
.build();
-
+ static final PropertyDescriptor PRIVATE_KEY = new
PropertyDescriptor.Builder()
+ .name("Private Key")
+ .description("Private RSA key generated foo Github App to use for
Authentication")
Review Comment:
Recommend the following wording adjustments, also noting that PKCS 8 is
required.
```suggestion
.description("RSA private key associated with GitHub App to use
for authentication. The private key must be PEM-encoded using PKCS 8.")
```
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubFlowRegistryClient.java:
##########
@@ -128,7 +128,22 @@ public class GitHubFlowRegistryClient extends
AbstractFlowRegistryClient {
.sensitive(true)
.dependsOn(AUTHENTICATION_TYPE,
GitHubAuthenticationType.APP_INSTALLATION_TOKEN.name())
.build();
-
+ static final PropertyDescriptor PRIVATE_KEY = new
PropertyDescriptor.Builder()
+ .name("Private Key")
+ .description("Private RSA key generated foo Github App to use for
Authentication")
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .required(true)
+ .sensitive(true)
+ .dependsOn(AUTHENTICATION_TYPE,
GitHubAuthenticationType.PRIVATE_KEY.name())
+ .build();
+ static final PropertyDescriptor APP_ID = new PropertyDescriptor.Builder()
+ .name("APP ID")
+ .description("App Id of Github App to use for Authentication")
Review Comment:
```suggestion
.description("Identifier of GitHub App to use for
authentication")
```
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java:
##########
@@ -35,10 +40,11 @@
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
-import java.util.List;
-import java.util.Objects;
-import java.util.Optional;
-import java.util.Set;
+import java.io.StringReader;
+import java.security.KeyFactory;
+import java.security.PrivateKey;
+import java.security.spec.PKCS8EncodedKeySpec;
+import java.util.*;
Review Comment:
Asterisk imports are not permitted in the the Checkstyle configuration.
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java:
##########
@@ -379,6 +392,41 @@ public GHContent deleteContent(final String filePath,
final String commitMessage
});
}
+ /**
+ * Creates the JwtToken for Authentication with Private Key
+ *
+ * @param pemString is the PKCS#1 String
+ * @return the JwtToken
+ *
+ * @throws Exception if an error occurs parsing key
+ */
+ private static String loadPrivateKeyFromPEM(String pemString) throws
Exception {
+ long nowMillis = System.currentTimeMillis();
+ long expMillis = nowMillis + 600000; // Token validity 10 minutes
+ Date now = new Date(nowMillis);
+ Date exp = new Date(expMillis);
+ PEMParser pemParser = new PEMParser(new StringReader(pemString));
+ Object object = pemParser.readObject();
+ pemParser.close();
+
+ if (object instanceof PEMKeyPair) {
+ PEMKeyPair keyPair = (PEMKeyPair) object;
+ RSAPrivateKey rsaPrivateKey =
RSAPrivateKey.getInstance(keyPair.getPrivateKeyInfo().parsePrivateKey());
+ PKCS8EncodedKeySpec keySpec = new
PKCS8EncodedKeySpec(rsaPrivateKey.getEncoded());
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ return (Jwts.builder().issuer(builder().appId)
+ .issuedAt(now)
+ .expiration(exp)
+ .signWith(privateKey,SignatureAlgorithm.RS256)
+ .compact());
+ } else {
+ LOGGER.error("Not a valid PKCS#1 PEM string");
+ }
+ LOGGER.warn("INVALID PEM KEY");
+ return "";
Review Comment:
This should throw an exception instead of logging a warning and returning an
empty string.
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubFlowRegistryClient.java:
##########
@@ -128,7 +128,22 @@ public class GitHubFlowRegistryClient extends
AbstractFlowRegistryClient {
.sensitive(true)
.dependsOn(AUTHENTICATION_TYPE,
GitHubAuthenticationType.APP_INSTALLATION_TOKEN.name())
.build();
-
+ static final PropertyDescriptor PRIVATE_KEY = new
PropertyDescriptor.Builder()
+ .name("Private Key")
+ .description("Private RSA key generated foo Github App to use for
Authentication")
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .required(true)
+ .sensitive(true)
+ .dependsOn(AUTHENTICATION_TYPE,
GitHubAuthenticationType.PRIVATE_KEY.name())
+ .build();
+ static final PropertyDescriptor APP_ID = new PropertyDescriptor.Builder()
+ .name("APP ID")
+ .description("App Id of Github App to use for Authentication")
+ .addValidator(StandardValidators.NON_BLANK_VALIDATOR)
+ .required(true)
+ .sensitive(true)
Review Comment:
The App ID is not sensitive, so this can be changed:
```suggestion
.sensitive(false)
```
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java:
##########
@@ -379,6 +392,41 @@ public GHContent deleteContent(final String filePath,
final String commitMessage
});
}
+ /**
+ * Creates the JwtToken for Authentication with Private Key
+ *
+ * @param pemString is the PKCS#1 String
+ * @return the JwtToken
+ *
+ * @throws Exception if an error occurs parsing key
+ */
+ private static String loadPrivateKeyFromPEM(String pemString) throws
Exception {
+ long nowMillis = System.currentTimeMillis();
+ long expMillis = nowMillis + 600000; // Token validity 10 minutes
+ Date now = new Date(nowMillis);
+ Date exp = new Date(expMillis);
+ PEMParser pemParser = new PEMParser(new StringReader(pemString));
+ Object object = pemParser.readObject();
+ pemParser.close();
+
+ if (object instanceof PEMKeyPair) {
+ PEMKeyPair keyPair = (PEMKeyPair) object;
+ RSAPrivateKey rsaPrivateKey =
RSAPrivateKey.getInstance(keyPair.getPrivateKeyInfo().parsePrivateKey());
+ PKCS8EncodedKeySpec keySpec = new
PKCS8EncodedKeySpec(rsaPrivateKey.getEncoded());
Review Comment:
We should use the utility from the github-api library, even though it is
limited to PKCS 8. This avoids the use of the Bouncy Castle library, and also
avoids the custom methods for token generation.
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubAuthenticationType.java:
##########
@@ -24,6 +24,7 @@ public enum GitHubAuthenticationType {
NONE,
PERSONAL_ACCESS_TOKEN,
- APP_INSTALLATION_TOKEN;
+ APP_INSTALLATION_TOKEN,
+ PRIVATE_KEY;
Review Comment:
Although `PRIVATE_KEY` is accurate from one perspective, it would be worth
indicating that this uses as a GitHub, so I recommend the following name:
```suggestion
APP_INSTALLATION_ID_AND_PRIVATE_KEY;
```
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java:
##########
@@ -74,6 +80,13 @@ private GitHubRepositoryClient(final Builder builder) throws
IOException, FlowRe
switch (authenticationType) {
case PERSONAL_ACCESS_TOKEN ->
gitHubBuilder.withOAuthToken(builder.personalAccessToken);
case APP_INSTALLATION_TOKEN ->
gitHubBuilder.withAppInstallationToken(builder.appInstallationToken);
+ case PRIVATE_KEY -> {
+ try {
+
gitHubBuilder.withJwtToken(loadPrivateKeyFromPEM(builder().privateKey));
+ } catch (Exception e) {
+ LOGGER.error("PEM Key or App Id is Invalid");
Review Comment:
The Client is not usable without proper authentication, so an exception
should be thrown instead of logging an error. It may be better to pass in the
private key instead of reading it in this class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
