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: issues-unsubscr...@nifi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org