Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Needs an uninstall #121

Open
trajano opened this issue Aug 8, 2020 · 7 comments
Open

Needs an uninstall #121

trajano opened this issue Aug 8, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@trajano
Copy link

trajano commented Aug 8, 2020

When starting this up using a ServletContextListener and terminating the application it says that there are ThreadLocals that are still remaining.

Calling Security.removeProvider doesn't resolve the threadlocal issues

public class InstallCorretto implements ServletContextListener {

    /**
     * Servlet context attribute key.
     */
    public static final String CONTEXT = "useCorretto";

    @Override
    public void contextDestroyed(@NotNull final ServletContextEvent sce) {

        Security.removeProvider(AmazonCorrettoCryptoProvider.PROVIDER_NAME);
        sce.getServletContext().removeAttribute(CONTEXT);

    }

    @Override
    public void contextInitialized(@NotNull final ServletContextEvent sce) {

        AmazonCorrettoCryptoProvider.install();
        sce.getServletContext().setAttribute(CONTEXT, true);

    }
}

Here's the output when terminating.
image

@SalusaSecondus
Copy link
Contributor

Our primary recommendation is to install ACCP at the server level rather than within a servlet or webapp. However, we will look into ways to address this.

Do you have any information on how other libraries have addressed this issue?

@trajano
Copy link
Author

trajano commented Aug 27, 2020

The best way I can think of is to create a webapp which loads up Cornetto as part of the servlet context
Then tell it to restart and see what is left over.

However, Security.removeProvider(AmazonCorrettoCryptoProvider.PROVIDER_NAME); should be the correct way of doing it.

@SalusaSecondus
Copy link
Contributor

Security.removeProvider() does uninstall the provider from the standard list. However it looks like all use of static ThreadLocals are problems for this use-case. I'll keep investigating, but I'm not certain the best method yet.

@trajano
Copy link
Author

trajano commented Aug 28, 2020

@SalusaSecondus
Copy link
Contributor

I have a few ideas of ways to fix this by eliminating most of my static fields which chain to thread locals (and am investigating them). However, instantiating cryptographic providers of any time (not just ACCP) can be computationally expensive. You really should try to install it once at the server level and leave it installed and alone.

@trajano
Copy link
Author

trajano commented Aug 28, 2020 via email

@robin-aws robin-aws added the enhancement New feature or request label Nov 27, 2020
@robin-aws
Copy link

Note I'm labelling this as "enhancement" given our recommendation against use cases that require uninstallation. It still may be better to use ThreadLocals in a friendlier way, but sounds like more investigation is still needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants