[ELY-1471] Rework the WildFlyElytronProvider to be a static definition instead of a dynamically generated definition. #1061
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Linux - JDK9 Build 133 outcome was FAILURE using a merge of 6a2c594 Failed tests |
hkalina
reviewed
Jan 3, 2018
| putService(new Service(this, SASL_SERVER_FACTORY_TYPE, SaslMechanismInformation.Names.IEC_ISO_9798_U_ECDSA_SHA1, "org.wildfly.security.sasl.entity.EntitySaslServerFactory", emptyList, emptyMap)); | ||
| putService(new Service(this, SASL_SERVER_FACTORY_TYPE, SaslMechanismInformation.Names.IEC_ISO_9798_M_ECDSA_SHA1, "org.wildfly.security.sasl.entity.EntitySaslServerFactory", emptyList, emptyMap)); | ||
| putService(new Service(this, SASL_SERVER_FACTORY_TYPE, SaslMechanismInformation.Names.EXTERNAL, "org.wildfly.security.sasl.external.ExternalSaslServerFactory", emptyList, emptyMap)); | ||
| putService(new Service(this, SASL_SERVER_FACTORY_TYPE, "GS2-KRB5-PLUS", "org.wildfly.security.sasl.gs2.Gs2SaslServerFactory", emptyList, emptyMap)); |
There was a problem hiding this comment.
Should not be constant Gs2.GS2_KRB5_PLUS used?
There was a problem hiding this comment.
That would cause the initialization of the Gs2 class, which may reintroduce the original problem.
dmlloyd
reviewed
Jan 3, 2018
| @@ -27,7 +27,7 @@ | |||
| /** | |||
| * SHA-512/256 hashing implementation as defined in FIPS PUB 180-4 Secure Hash Standard | |||
| */ | |||
| public class Sha512_256MessageDigest extends MessageDigestSpi { | |||
| public class SHA512_256MessageDigest extends MessageDigestSpi { | |||
There was a problem hiding this comment.
I probably should have put that in it's own commit but it popped up in the middle of this clean up - a while ago we agreed to upper case 3 letter acronyms and camel case 4 or more - this is a new class added in Elytron 1.2 so we can get away with a quick rename before it leaks into any API.
hkalina
approved these changes
Jan 3, 2018
dmlloyd
reviewed
Jan 3, 2018
| putService(new Service(this, ALG_PARAMS_TYPE, ALGORITHM_MASKED_PBKDF_HMAC_SHA512, MaskedPasswordAlgorithmParametersSpiImpl.class.getName(), emptyList, emptyMap)); | ||
| putService(new Service(this, ALG_PARAMS_TYPE, "RSA", "org.wildfly.security.key.RSAParameterSpiImpl", emptyList, emptyMap)); | ||
|
|
||
| putService(new Service(this, ALG_PARAMS_TYPE, ALGORITHM_CRYPT_MD5, "org.wildfly.security.password.impl.SaltedPasswordAlgorithmParametersSpiImpl", emptyList, emptyMap)); |
There was a problem hiding this comment.
A more efficient approach might be to read all this info from a text file to avoid all these strings sticking around in metaspace forever.
There was a problem hiding this comment.
Which Strings are you worried about @dmlloyd - The referenced constants or the hard coded ones?
At this stage it felt as though any over optimisation here will be undone as soon as Elytron is actually used one way or another, also although we can use files it seems we would be likely to undo any savings with the classes we load to read the files.
…n instead of a dynamically generated definition. This drastically reduces the class loading overhead just to initialise the Provider.
dmlloyd
approved these changes
Feb 8, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This drastically reduces the class loading overhead just to initialise the Provider.