Skip to content

enhancement(5039): bump up go version to 1.24, replace pbkdf2 with stdlib pbkdf2#291

Closed
kaanyalti wants to merge 1 commit into
elastic:mainfrom
kaanyalti:enhancement/5039_remove_x_crypto_fips
Closed

enhancement(5039): bump up go version to 1.24, replace pbkdf2 with stdlib pbkdf2#291
kaanyalti wants to merge 1 commit into
elastic:mainfrom
kaanyalti:enhancement/5039_remove_x_crypto_fips

Conversation

@kaanyalti
Copy link
Copy Markdown

  • Enhancement

What does this PR do?

Bumps up the go version to 1.24 and replaces x/crypto/pbkdf2 with stdlib pbkdf2.

Why is it important?

We are moving away from using x/crypto to be fips compliant

Checklist

  • My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have added tests that prove my fix is effective or that my feature works

Related issues

@kaanyalti kaanyalti requested a review from a team as a code owner March 6, 2025 05:13
@kaanyalti kaanyalti requested review from faec and khushijain21 and removed request for a team March 6, 2025 05:13
Comment thread go.mod
module github.com/elastic/elastic-agent-libs

go 1.22.12
go 1.24
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

go 1.23 is still supported, we can't drop it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we still need to support go.1.23 here then we won't be able to move from golang.org/x/crypto/pbkdf2 to the stdlib crypto implementations.

@kaanyalti @kruskall I wonder why the keystore even shows up as an issue in the downstream dependencies since it was excluded when in fips mode.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should refrain from updating the minimum Go version in libs until the respective update for Beats/Agent is well underway. Sometimes, we have important fixes in libs that need to be backported to Beats/Agent. If we update this too soon, we risk soft-locking ourselves out of those important changes.

That said, if we first update Beats/Agent to 1.24 and it goes well, I see no reason why we cannot make the move to update libs too.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we still need to support go.1.23 here then we won't be able to move from golang.org/x/crypto/pbkdf2 to the stdlib crypto implementations.

we can use x/crypto for go <1.24 and stdlib for go >=1.24. See #289

I wonder why the keystore even shows up as an issue in the downstream dependencies since it was excluded when in fips mode.

If the package is imported then it is linked in the final binary (especially with DCE disabled). It's one of the reason the kerberos library was excluded in a way that avoided importing the package.

That said, if we first update Beats/Agent to 1.24 and it goes well, I see no reason why we cannot make the move to update libs too.

I'm not sure I agree :(
This is a library and it's not only used by beats/agent. The policy has usually been to support all the supported versions of go in libraries (currently 1.23 and 1.24)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a library and it's not only used by beats/agent. The policy has usually been to support all the supported versions of go in libraries (currently 1.23 and 1.24)

Fair point, agree with you on this.

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 6, 2025

💔 Build Failed

Failed CI Steps

History

Copy link
Copy Markdown
Member

@kruskall kruskall left a comment

Choose a reason for hiding this comment

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

see #289

@simitt
Copy link
Copy Markdown
Contributor

simitt commented Mar 7, 2025

I believe this is superseded and fixed by #289

@kaanyalti
Copy link
Copy Markdown
Author

Thank you all for the input, closing this PR as it is not needed

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.

5 participants