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

ESLint Rule to discourage hashes being created with unsafe algorithms #190973

Merged
merged 34 commits into from
Sep 30, 2024

Conversation

kc13greiner
Copy link
Contributor

@kc13greiner kc13greiner commented Aug 21, 2024

Closes #185601

Summary

Using non-compliant algorithms with Node Cryptos createHash function will cause failures when running Kibana in FIPS mode.

We want to discourage usages of such algorithms.

schema: [],
},
create(context) {
const allowedAlgorithms = ['sha1', 'sha256'];
Copy link
Contributor

Choose a reason for hiding this comment

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

A list of allowed algorithms to be used with crypto.createHash

@SiddharthMantri SiddharthMantri changed the title Creating an ES Lint Rule to discourage hashes being created with MD5 Creating an ES Lint Rule to discourage hashes being created with unsafe algorithms Aug 30, 2024
@SiddharthMantri SiddharthMantri changed the title Creating an ES Lint Rule to discourage hashes being created with unsafe algorithms ES Lint Rule to discourage hashes being created with unsafe algorithms Aug 30, 2024
@SiddharthMantri SiddharthMantri marked this pull request as ready for review August 30, 2024 08:28
@SiddharthMantri SiddharthMantri requested review from a team as code owners August 30, 2024 08:28
@SiddharthMantri
Copy link
Contributor

@elasticmachine merge upstream

schema: [],
},
create(context) {
const allowedAlgorithms = ['sha1', 'sha256', 'sha3-256'];
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks! Addressed in 79678e8

@jcger
Copy link
Contributor

jcger commented Aug 30, 2024

Is this pinging response-ops because there is a rules folder in it?

@SiddharthMantri
Copy link
Contributor

Is this pinging response-ops because there is a rules folder in it?

@jcger That's probably it. Not sure where the automation picked up response-ops from. Sorry for the noise!

@SiddharthMantri SiddharthMantri requested review from jbudz and removed request for a team August 30, 2024 18:45
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

Looks fine, probably want to add sha512 and open issues / eslint ignore the remainder of the lint errors

@SiddharthMantri SiddharthMantri requested review from a team as code owners September 2, 2024 08:58
@SiddharthMantri
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

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

Code Review only, lgtm

@SiddharthMantri
Copy link
Contributor

@elastic/security-scalability Hi team! We need your review on this PR. I think you were added as a result of a bad merge with main somewhere, so I don't think there are any files owned by you. But just in case, please review and approve when possible.

@SiddharthMantri SiddharthMantri changed the title ES Lint Rule to discourage hashes being created with unsafe algorithms ESLint Rule to discourage hashes being created with unsafe algorithms Sep 25, 2024
@ilyannn ilyannn self-requested a review September 25, 2024 10:49
Copy link
Contributor

@ilyannn ilyannn left a comment

Choose a reason for hiding this comment

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

Please take a closer look at the scalability-owned changes

@@ -4,7 +4,7 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
/* eslint-disable @typescript-eslint/no-explicit-any */

Copy link
Contributor

Choose a reason for hiding this comment

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

This file is indeed owned by @elastic/security-scalability .

Unfortunately, it makes no sense to accept your suggestion as this rules has been disabled for a reason. We get three warnings if we disable the rule for this file:

image

Let's revert this:

Suggested change
/* eslint-disable @typescript-eslint/no-explicit-any */

Copy link
Contributor

@SiddharthMantri SiddharthMantri Sep 25, 2024

Choose a reason for hiding this comment

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

Thanks @ilyannn. Looks like change was introduced in a bad merge. Reverted now.

@SiddharthMantri
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@ilyannn ilyannn left a comment

Choose a reason for hiding this comment

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

No issues from scalability now 😄

@SiddharthMantri SiddharthMantri enabled auto-merge (squash) September 25, 2024 11:05
@SiddharthMantri SiddharthMantri enabled auto-merge (squash) September 25, 2024 11:06
@SiddharthMantri
Copy link
Contributor

@elasticmachine merge upstream

@SiddharthMantri
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 30, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/core-apps-server-internal 0 1 +1
@kbn/core-rendering-server-internal 0 1 +1
@kbn/core-test-helpers-so-type-serializer 0 1 +1
@kbn/es 2 4 +2
@kbn/failed-test-reporter-cli 0 1 +1
@kbn/optimizer 1 2 +1
@kbn/test-suites-xpack 720 721 +1
cloudFullStory 1 2 +1
dataViews 9 10 +1
total +10

Total ESLint disabled count

id before after diff
@kbn/core-apps-server-internal 1 2 +1
@kbn/core-rendering-server-internal 2 3 +1
@kbn/core-test-helpers-so-type-serializer 0 1 +1
@kbn/es 2 4 +2
@kbn/failed-test-reporter-cli 1 2 +1
@kbn/optimizer 3 4 +1
@kbn/test-suites-xpack 744 745 +1
cloudFullStory 2 3 +1
dataViews 11 12 +1
total +10

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@SiddharthMantri SiddharthMantri merged commit f207c2c into elastic:main Sep 30, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project Feature:FIPS FIPS mode for Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Obs AI Assistant Observability AI Assistant Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:obs-ux-management Observability Management User Experience Team Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add checks to enforce/discourage further usage of MD5