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

Downgrade cron-utils to 9.2.0 #873

Merged
merged 2 commits into from
Aug 28, 2023
Merged

Downgrade cron-utils to 9.2.0 #873

merged 2 commits into from
Aug 28, 2023

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Aug 26, 2023

Motivation:
We have upgraded cron-utils to 9.2.1 to address CVE-2021-41269. The 9.2.1 version uses slf4j2 that must be used with logback 1.3 or 1.4. Because we use logback 1.2 version in Armeria, we need to exclude slf4j2: #872.
Just excluding slf4j should be okay because cron-utils isn't using any APIs that are introduced after slf4j 2.0: https://github.com/search?q=repo%3Ajmrozanec%2Fcron-utils+slf4j&type=code
However, there's no guarantee that cron-utils won't use the new APIs in the future. So, I think we should stop upgrading it until there's another CVE is found or Armeria uses higher version of Logback and Slf4j.\

While, I'm working on this I found out that cron-utils 9.2.0, which is one micro version eariler, uses Slf4j 1.x which is compatible with Armeria so it's better to use that version.

Modification:

  • Downgrade cron-utils to 9.2.0

Result:

  • Resovle dependency conflict for server module.

Motivation:
We have upgraded cron-utils to 9.2.1 to address [CVE-2021-41269](GHSA-p9m8-27x8-rg87).
The 9.2.1 version uses slf4j2 that must be used with logback 1.3 or 1.4.
Because we use logback 1.2 version in Armeria, we need to exclude slf4j2: line#872
Just excluding slf4j should be okay because cron-utils isn't using any APIs that are introduced after slf4j 2.0:
https://github.com/search?q=repo%3Ajmrozanec%2Fcron-utils+slf4j&type=code
However, there's no guarantee that cron-utils won't use the new APIs in the future.
So, I think we should stop upgrading it until there's another CVE is found or Armeria uses higher version of Logback and Slf4j.\

While, I'm working on this I found out that cron-utils 9.2.0, which is one micro version eariler, uses Slf4j 1.x which is compatible with Armeria
so it's better to use that version.

Modification:
- Downgrade cron-utils to 9.2.0

Result:
- Resovle dependency conflict for server module.
@minwoox
Copy link
Member Author

minwoox commented Aug 26, 2023

I have checked that there's no SLF4J2 in the classpath for the server.

@@ -37,6 +37,7 @@
<logger name="com.github.benmanes.caffeine.cache" level="ERROR" />
<logger name="com.linecorp" level="INFO" />
<logger name="com.linecorp.centraldogma" level="DEBUG" />
<logger name="com.linecorp.centraldogma.server.internal.mirror" level="INFO" />
Copy link
Member Author

@minwoox minwoox Aug 26, 2023

Choose a reason for hiding this comment

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

I found out that GitSshdSessionFactory from Apache MINA, which is Inherited by the DefaultGitSshdSessionFactory Central Dogma class, produces verbose debug outputs.
GitSshdSessionFactory creates the logger with its class name, which is DefaultGitSshdSessionFactory in this case, so they are all logged by the <logger name="com.linecorp.centraldogma" level="DEBUG" />.

2023-08-26 00:38:06.468 [DEBUG](c.l.c.s.i.m.GitWithAuth$DefaultGitSshdSessionFactory$1) [mirroring-worker-6-1] Connecting to ...
2023-08-26 00:38:06.537 [DEBUG](c.l.c.s.i.m.GitWithAuth$DefaultGitSshdSessionFactory$1) [mirroring-worker-6-1] Connected to ...
2023-08-26 00:38:06.537 [DEBUG](c.l.c.s.i.m.GitWithAuth$DefaultGitSshdSessionFactory$1) [mirroring-worker-6-1] Authenticating: ClientSessionImpl[git@...]
.....
6-1] Stopped SshClient[3c45db89]

Copy link
Member

Choose a reason for hiding this comment

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

Can we extract the class (...$1) into a top-level class and adjust the log level for that specific class only?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two separate DefaultGitSshdSessionFactory anonymous classes in the file.
Let me just use the class specifically:
<logger name="com.linecorp.centraldogma.server.internal.mirror.GitWithAuth$DefaultGitSshdSessionFactory" level="INFO" />

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

@codecov
Copy link

codecov bot commented Aug 26, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.03% ⚠️

Comparison is base (6b10542) 65.69% compared to head (be016dc) 65.66%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #873      +/-   ##
============================================
- Coverage     65.69%   65.66%   -0.03%     
+ Complexity     3350     3349       -1     
============================================
  Files           358      358              
  Lines         13893    13893              
  Branches       1496     1496              
============================================
- Hits           9127     9123       -4     
- Misses         3914     3918       +4     
  Partials        852      852              

see 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jrhee17 jrhee17 added this to the 0.62.1 milestone Aug 28, 2023
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! 🙇‍♂️

@jrhee17 jrhee17 merged commit 641c309 into line:main Aug 28, 2023
11 checks passed
@minwoox minwoox deleted the cron-utils-down branch August 28, 2023 03:42
@minwoox
Copy link
Member Author

minwoox commented Aug 28, 2023

Thanks for the review and sorry for causing the issue. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants