#782: Make runtime registration of bouncycastle skippable #828
Conversation
|
|
| @@ -275,17 +275,20 @@ public static synchronized void setSecurityProvider(String securityProvider) { | |||
| } | |||
|
|
|||
| private static void register() { | |||
| if (!registrationDone) { | |||
| String noBCRegistrationAtRuntime = System.getProperty("bouncycastle.registration.runtime"); | |||
There was a problem hiding this comment.
Instead of doing it like this, let's make it just a configuration in the config object. This will allow a user of the library to choose how to configure it, either through an env variable or through his own config.
Now we would be forcing them to set an env variable.
There was a problem hiding this comment.
Thanks for the quick feedback! 👍 Please take another look.
| Properties properties = new Properties(); | ||
| properties.load(DefaultConfig.class.getClassLoader().getResourceAsStream("sshj.properties")); | ||
| String property = properties.getProperty("sshj.bouncycastle.runtime.registration"); |
There was a problem hiding this comment.
This approach requires reading a file from the class path, it seems like it would be better to avoid the lookup and make this a simple member variable of the DefaultConfig class.
There was a problem hiding this comment.
Its already possible to set the variable programmatically by using the DefaultConfig.
def config = new DefaultConfig()
config.setBouncycastleRuntimeRegistrationEnabled(true)
With my approach you can additionally add it via properties file. As @hierynomus already mentioned, this way the user can decide for himself what to use.
There was a problem hiding this comment.
Thanks for the clarification, that makes sense.
There was a problem hiding this comment.
I think you've misunderstood. It's up to the application that integrates sshj to decide how to pass the config to sshj. This kind of property or env reading code does not belong in the lib but rather in the surrounding app
There was a problem hiding this comment.
Thanks for the feedback. I actually misunderstood. I have taken out the property so that you can only set it via the DefaultConfig. Any other feedback you have?
| @@ -73,6 +73,7 @@ | |||
| public DefaultConfig() { | |||
| setLoggerFactory(LoggerFactory.DEFAULT); | |||
| setVersion(readVersionFromProperties()); | |||
| SecurityUtils.setBouncycastleRuntimeRegistration(isBouncycastleRuntimeRegistrationEnabled()); | |||
There was a problem hiding this comment.
If you would add some tests, you'd see that you can never ever change the configuration flag, as it is already passed down in the constructor of the config if you do it like this.
There was a problem hiding this comment.
Thanks, you are right. Will address it asap
|
The current approach disables the bouncycastle runtime registration using the defaultconfig, but it does not work for GraalVM Native Image because the defaultconfig is called too late at runtime. |
|
New approach to skip the bouncycastle registration at runtime is to use a system property set by GraalVM. This is also used by Spring to determine if a native image is running. |
|
I will reiterate on what I've said:
|
|
@Hayvon, I opened pull request #861 that attempts to address the Bouncy Castle registration issue. Registration can be disabled in the current version of SSHJ using the |
|
@Hayvon and @hierynomus is this pull request still necessary? With the changes in PR #861 supporting SSHJ configuration with Bouncy Castle registration, the changes here no longer appear to be required. |
|
Indeed, I'll close this PR as the changes have been addressed in a better way in #861 |
Adressing #782 by adding a system property check which makes the runtime registration of bouncycastle skippable.