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

feat(inputs.x509_cert): Add support for JKS and PKCS#12 keystores #16508

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

paulojmdias
Copy link
Contributor

Summary

I initially thought about creating a separate input plugin for JKS & PKCS12 key stores but decided to integrate it into x509_cert since the changes were minimal. This update allows x509_cert to automatically detect and parse JKS & PKCS12 files while maintaining the existing certificate validation and metric collection logic. I also added tests to ensure everything works smoothly without overcomplicating the code.

Please evaluate if make sense and let me know what changes you feel are needed to accommodate this feature.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #7013

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Feb 12, 2025
@srebhan
Copy link
Member

srebhan commented Feb 12, 2025

@paulojmdias please check the linter issues. You can reproduce them locally using make check-deps.

@srebhan srebhan self-assigned this Feb 12, 2025
@paulojmdias
Copy link
Contributor Author

@paulojmdias please check the linter issues. You can reproduce them locally using make check-deps.

Fixed 🙌

@paulojmdias paulojmdias changed the title feat(inputs.x509_cert): add support for JKS and PKCS#12 keystors feat(inputs.x509_cert): add support for JKS and PKCS#12 keystores Feb 13, 2025
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @paulojmdias! This is very much appreciated! I do have some comments in the code...

@srebhan srebhan changed the title feat(inputs.x509_cert): add support for JKS and PKCS#12 keystores feat(inputs.x509_cert): Add support for JKS and PKCS#12 keystores Feb 17, 2025
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

@paulojmdias some more comments...


_, cert, caCerts, err := pkcs12.DecodeChain(data, passwordStr)
if err != nil {
_, cert, caCerts, err = pkcs12.DecodeChain(data, "") // Retry without password
Copy link
Member

Choose a reason for hiding this comment

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

Why is this required? The user can specify an empty password if he needs to, so don't try to be smart here as this usually calls for trouble. ;-)

Copy link
Contributor Author

@paulojmdias paulojmdias Feb 18, 2025

Choose a reason for hiding this comment

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

Make sense to set the default password as "" in Init() instead of the user explicitly adding it to the configuration? Or by default is "" if is not set?

err = jks.Store(output, []byte("test-password"))
require.NoError(t, err)

return "jks://" + jksPath
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work on Windows, you should do

Suggested change
return "jks://" + jksPath
jksPath = filepath.ToSlash(jksPath)
if !strings.HasPrefix(jksPath, "/") {
jksPath = "/" + jksPath
}
return "jks://" + jksPath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed also pkcs12Path

Copy link
Contributor Author

@paulojmdias paulojmdias Feb 18, 2025

Choose a reason for hiding this comment

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

This still seems to be not working on Windows. The code below will not make it work using absolute paths?

absPath, err := filepath.Abs(jksPath)
require.NoError(t, err)

return "jks://" + absPath

Signed-off-by: Paulo Dias <[email protected]>
Signed-off-by: Paulo Dias <[email protected]>
Signed-off-by: Paulo Dias <[email protected]>
@telegraf-tiger
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for JKS files to x509_cert plugin
2 participants