Skip to content

Move library specific logic to the backend#1283

Open
KostasTsiounis wants to merge 1 commit intoIBM:mainfrom
KostasTsiounis:move_context
Open

Move library specific logic to the backend#1283
KostasTsiounis wants to merge 1 commit intoIBM:mainfrom
KostasTsiounis:move_context

Conversation

@KostasTsiounis
Copy link
Copy Markdown
Member

Library specific logic, such as contexts and choice of FIPS or non-FIPS binaries, is performed in the provider.

This change moves said logic and functionality to the backend, in order to decouple it and allow for introduction of different libraries seamlessly.

Signed-off-by: Kostas Tsiounis kostas.tsiounis@ibm.com

@johnpeck-us-ibm
Copy link
Copy Markdown
Member

Looking at the code. I think it can be made a little simpler. The only difference between FIPS and non-FIPS is the context. So, there really is no need for a FIPS version and non-FIPS version. Update the provider to include isFIPS() which I think was done and use that for creating context and which context to use.

The other thing that we need to look at is the cleanup stuff in the provider. The question will be if we need to move this to the native provider implementation. Because at some point what needs to be done for ICC, may not be what needs to be done for OpenSSL.

@KostasTsiounis
Copy link
Copy Markdown
Member Author

Looking at the code. I think it can be made a little simpler. The only difference between FIPS and non-FIPS is the context. So, there really is no need for a FIPS version and non-FIPS version. Update the provider to include isFIPS() which I think was done and use that for creating context and which context to use.

I'm not sure if it would be simpler. The NativeOCKAdapter class is doing what you suggest, which is get a boolean isFIPS and create the appropriate context. But instead of keeping both contexts in the same class, I have added 2 subclasses, one for each type (FIPS or non-FIPS). This allows us to do 2 things:

  1. Have specific code like the developer mode warnings.
  2. Avoid checking for the type of context every time. For example, if one wants to do RSA cipher operations, they choose FIPS or non-FIPS in the constructor and then just call the appropriate methods. If I only used one class, we would have to pass the boolean parameter in every call and have an if statement to decide on the context every single time.

I think this way a better separation of concerns is created and we avoid additional checks. Unless I'm missing something and you have another way to do it that I haven't considered of course.

The other thing that we need to look at is the cleanup stuff in the provider. The question will be if we need to move this to the native provider implementation. Because at some point what needs to be done for ICC, may not be what needs to be done for OpenSSL.

I'm guessing you're talking about the Cleaner instances and actions, right? Well, OpenSSL will probably need this since it will also have native memory to clear, but other stuff like FFM might not. So, you're right in that we could move it. I'm not sure we want to do it as part of this PR though, so as to not further complicate it, but I'm open to further discussing it.

Copy link
Copy Markdown
Member

@johnpeck-us-ibm johnpeck-us-ibm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with these changes

Copy link
Copy Markdown
Member

@jasonkatonica jasonkatonica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Library specific logic, such as contexts and choice of FIPS or
non-FIPS binaries, is performed in the provider.

This change moves said logic and functionality to the backend,
in order to decouple it and allow for introduction of different
libraries seamlessly.

Signed-off-by: Kostas Tsiounis <kostas.tsiounis@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants