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

Thales cipher trust kms plugin #4589

Closed
wants to merge 14 commits into from

Conversation

remythales
Copy link

@remythales remythales commented Oct 20, 2023

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
Key Manager (new)

Description of change
Added Built-In plugin to support Thales CipherTrsut KMS

Which issue this PR fixes
None

@remythales
Copy link
Author

remythales commented Oct 26, 2023

Hi Evan,
I have an a business case with HPE Greenlake and we had a request to integrate with Thales Key Manager system.
Do let me know if you have any concerns.

Thanks a lot for your time
Remy

@remythales
Copy link
Author

remythales commented Nov 2, 2023

Just to add some info here: The plugin can be used in the free community version of CipherTrust Manager. This is to extend the CTM feature (non-commercial).

@evan2645
Copy link
Member

evan2645 commented Nov 6, 2023

Hi @remythales , very sorry for the delay here, thanks for your patience.

As I mentioned briefly on Slack last week, we need some help formalizing a policy around if/how the project can accept plugins for non-free commercial software/platforms/vendors. There are a few challenges to figure out there, and we've captured them in this issue. IMO, it is beneficial for the project to have such plugins available out-of-the-box, however we need to make sure that we do it in a sustainable way.

In a past PR, the one which triggered the creation of this issue, the author published the plugins on their own as out-of-tree plugins. The reality is, it would be helpful to have an interested vendor participating in the conversation on #4163 so we can arrive at a solution that makes sense to both parties. Please have a look and let us know what you think ... I'm afraid it will be difficult for us to accept these patches without some decisions there first

@evan2645
Copy link
Member

Hi @remythales - just wanted to follow up from our conversation last week. In order to get this merged, all we need is:

  • At least one integration test exercising the functionality against your community edition software
  • Names of at least two people who are willing to act as subsystem maintainer for the plugin

I will update the project documentation to outline this new policy, and will also add the provided names to the project's CODEOWNERS file. Please let us know if you run into any problems with these points. We're happy to help you get an out-of-tree plugin published as well in the event you hit any blockers

@remythales
Copy link
Author

Hi @evan2645 ,
I have forwarded your request internally and will update yon once I reply.
Thanks for your help and understanding

@evan2645
Copy link
Member

evan2645 commented Jan 2, 2024

Hi @remythales, happy new year 😁

We are planning to close this PR next week if we don't have a path forward. Happy to provide some examples of how to host and ship out-of-tree plugins for SPIRE if that's helpful in the interim

@remythales
Copy link
Author

Thanks :) Happy New Year to you, too.
The team was on holiday and will be back shortly. I will post any news once I hear back from them.
Thanks for your understanding.

@remythales
Copy link
Author

remythales commented Jan 9, 2024

Hi Evan,

We do have the integrated test suite inside the plugin, and we can provide you with the names of the maintainers. I am waiting for new credentials for you to run the integration test suite.

Thanks

@remythales
Copy link
Author

Hi Evan,

Sorry for not getting back to you sooner, but I managed to pull off some info for you to complete the request.

Here is the first name for the plugin: Anurag JAIN - [email protected]
I'm looking for another name to give you; we could move forward this way._

The test suite has already been developed as part of the plugin testing, but it needs the KMS, as I describe below.

There is a community edition option available here - community version
You can run locally on virtualbox or vsphere or even from cloud marketplaces.
A couple of videos to setup Ciphertrust Manager:
Locally
Cloud

The capabilities like key management are available for free forever (What we use in the plugin), and there is a 90-day evaluation option (For advanced features that we don't use in the plugin) that can be enabled from CM GUI.

For key management, you may also use the 30-day free trial of CipherTrust as a Service

Thanks,
Remy

@evan2645
Copy link
Member

Great news, thank you @remythales! Can you please provide the GitHub handle for Anurag Jain? Once you have the other person, we will set up a Zoom call to make sure we're all aligned on the needs and what it means to maintain this code etc

As for the integration test, I think it makes sense for the plugin to support anything we can reliably test against. In this case, that might mean supporting only what is available in the free version? The integration test needs to be automated and checked into our integration test suite as part of this PR

@remythales
Copy link
Author

Excellent, I have contacted the concerned party to have their github handle to me.
Please allow me some time, I will be reaching back as soon as I have the info.

Yes, you are right, the plugin only your the free feature of the CipherTrust Manager KMS.

Thanks for your time.

@evan2645
Copy link
Member

Ok thank you @remythales! And just to highlight this, the current PR will need to be updated to include the integration test before we can review it 🙏

@remythales
Copy link
Author

My colleague's GitHub handles is @anugram the associated email is [email protected].
The integration test is already part of the PR: pkg/server/plugin/keymanager/ciphertrustkms/ciphertrustkms_test.go

For the second name needed, you can use mine @remythales

As for now, I would need to move to other hot customer topics, no further development will be done on my side for this PR request. So that we can proceed to close this subject.

Thanks a lot of your help and patience.

@evan2645
Copy link
Member

evan2645 commented Feb 6, 2024

My colleague's GitHub handles is @anugram the associated email is [email protected].
For the second name needed, you can use mine @remythales

Ok great! Thank you 🙏

The integration test is already part of the PR: pkg/server/plugin/keymanager/ciphertrustkms/ciphertrustkms_test.go

These tests are great and we should definitely keep them, but I am talking about functional integration testing using the test suite I linked earlier: https://github.com/spiffe/spire/tree/main/test/integration

A test there for this plugin would be able to fetch the community edition of the kms software, load it against a built and running SPIRE Server, and exercise the various pathways to ensure that everything works as expected.

As for now, I would need to move to other hot customer topics, no further development will be done on my side for this PR request. So that we can proceed to close this subject.

I'm sorry to hear that, but totally understand. I think this is a good time to highlight the ongoing nature of maintaining upstream source code - there's a general expectation of timely response (especially for security related issues), and so I would hope that your leadership is able to make this time for you and @anugram to support the contribution on an ongoing basis. I previously mentioned that it would be good to jump on a quick zoom call when the time is right, and this is one of the topics I was hoping to discuss at that time.

As for this PR, my sense is that we need a functional integration test as part of this PR in order to feel comfortable taking the contribution. I realize that it's a non-trivial amount of work. I hope you're able to find support for completing it, as well as making the necessary time to maintain the plugin in the long run (though I expect that commitment would be minimal).

Please let me know if it's something you think you'll be able to complete in the next few weeks. Otherwise, I can close this PR out for now and we can re-visit when you have more cycles. Thanks for understanding

@remythales
Copy link
Author

remythales commented Feb 7, 2024

excellent,

I will tackle the integration test (Functional) next week.

we will get to the bottom of this; thanks a lot for your insight and patience.

@amartinezfayo
Copy link
Member

Hi @remythales, I think that it would be good to have a call about this contribution, so we can align on the needs and expectations. Could you please share your preferred e-mail address and some options that would work for you/your team to have a Zoom call? Thanks!

@remythales
Copy link
Author

Sure, here is my Thales email.
[email protected]

thanks

@remythales
Copy link
Author

Hi @evan2645,

I have pushed the integration testing.
After discussing with the SPIRE team today and the CipherTrust Thales team, I realized that having CipherTrust Key Manager into a docker for CI/CD is not possible.

I understand the Spire team doesn't want to have external API calls, but it is a faster and easier way of testing. Nevertheless, one option would be to deploy an instance of CipherTrust Key Manager in a Virtual Box CipherTrust Local VM installation.

It works fine, and the integration testing was successful, the configuration of the plugin is now with a local IP address.

looking forward to hearing from you.
Remy

@amartinezfayo
Copy link
Member

Hi @remythales, unfortunately, we cannot have an instance of CipherTrust Key Manager in a Virtual Box integrated in our GitHub workflows. We don't have a way to do that.
Exercising the integration tests as part of our CI/CD without performing external API calls is a requirement, and it doesn't seem that there is a viable solution that could be integrated in our GitHub workflows (e.g. using Docker images).
In the absence of a solution at this point in time, our recommendation is to publish it as an external plugin in a separate GitHub repository that you control. External plugins can be configured using the plugin_cmd and plugin_checksum config settings in the Plugin configuration. We could link to the external plugin in places like https://github.com/spiffe/awesome-spiffe.
I know that it may be frustrating, but it seems to be the only solution at this moment.

@remythales
Copy link
Author

remythales commented Feb 28, 2024

Understood.

I have checked the documentation for authoring a plugin few things are not clear:

  • The bootstrap of the plugin initialization.
    -> The Serve function
    -> The Main function seems to be in a different package. Do I call the Serve function from there?

Do I need the catalog for the plugin?
I used to have from the catalog the builtin function that takes care of the plugin initialization with the Plugin construction quite straight forward.

I think a flow diagram on how to bootstrap the plugin provided in the template folder could really help.

I have taken example from the TPM plugin but seems quite old now the interface to initialize the plugin seems not to be working anymore.

thanks.

@amartinezfayo
Copy link
Member

Changes in the catalog are not needed, all the changes are self-contained in the external plugin code. No changes in SPIRE will be required.
To author an external plugin, the only difference is that you provide a main function that invokes the pluginmain.Serve function. The spire-plugin-sdk repository has a template demonstrating this as part of the KeyManager plugin template: https://github.com/spiffe/spire-plugin-sdk/blob/main/templates/server/keymanager/keymanager.go

Then, you build the plugin code to have a binary that you can reference in the plugin_cmd setting. Just make sure that you have a main package with the main function so you can compile the plugin having the resulting object binary that you set in plugin_cmd.

    KeyManager "ciphertrust_kms" {
        plugin_cmd = "path/to/ciphertrust_kms"
        #  plugin_checksum = "An optional sha256 of the plugin binary"
        plugin_data = {
            # Plugin settings
        }
    }

@amartinezfayo
Copy link
Member

Since this will be published as an external plugin, I'll go ahead and close this PR. Please reach out if you need any help.

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