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

Prohibit mirroring to internal repositories #621

Merged
merged 3 commits into from Aug 11, 2021

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Aug 10, 2021

Motivation:
We should prohibit mirroring to internal repositories which can cause a security incident.

Modification:

  • Raise an exception if the localRepo of mirroring setting is one of meta and dogma which are internal repositories.

Result:

  • You cannot set up mirroring to internal repositories anymore.

Motivation:
We should prohibit mirroring to internal repositories which can cause a security incident.

Modifcations:
- Raise an exception if the `localRepo` of mirroring setting is one of `meta` and `dogma` which are internal repositories.

Result:
- You cannot setup mirroring to internal repositories anymore.
@minwoox minwoox added the defect label Aug 10, 2021
@minwoox minwoox added this to the 0.52.0 milestone Aug 10, 2021
@codecov
Copy link

codecov bot commented Aug 10, 2021

Codecov Report

Merging #621 (e83b558) into master (ed507c7) will increase coverage by 0.12%.
The diff coverage is 75.75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #621      +/-   ##
============================================
+ Coverage     69.79%   69.91%   +0.12%     
- Complexity     3262     3276      +14     
============================================
  Files           331      331              
  Lines         12988    13020      +32     
  Branches       1396     1402       +6     
============================================
+ Hits           9065     9103      +38     
+ Misses         3062     3056       -6     
  Partials        861      861              
Impacted Files Coverage Δ
...rnal/storage/repository/DefaultMetaRepository.java 86.77% <ø> (ø)
...erver/internal/thrift/CentralDogmaServiceImpl.java 74.65% <62.50%> (-0.59%) ⬇️
...raldogma/server/internal/api/ContentServiceV1.java 84.49% <80.00%> (-1.09%) ⬇️
...ient/armeria/legacy/LegacyCentralDogmaBuilder.java 81.48% <0.00%> (-3.71%) ⬇️
...al/storage/repository/cache/CachingRepository.java 89.31% <0.00%> (+0.76%) ⬆️
...internal/storage/DirectoryBasedStorageManager.java 65.06% <0.00%> (+0.87%) ⬆️
.../centraldogma/internal/client/AbstractWatcher.java 80.68% <0.00%> (+2.75%) ⬆️
...a/server/internal/api/HttpApiExceptionHandler.java 84.00% <0.00%> (+4.00%) ⬆️
...ogma/client/armeria/CentralDogmaEndpointGroup.java 65.38% <0.00%> (+7.69%) ⬆️
...aldogma/client/armeria/CompositeEndpointGroup.java 100.00% <0.00%> (+18.75%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed507c7...e83b558. Read the comment docs.

* is one of {@code meta} and {@code dogma} which are internal repositories.
*/
private void checkMirrorLocalRepo(Repository repository, Iterable<Change<?>> changes) {
// TODO(minwoox): Provide an internal API for mirroring setup with a better UI(?) and check this there.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

final Object content = change.content();
if (content != null && content instanceof JsonNode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

change.content() seems always not null because of the previous filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed. 😉

@@ -177,6 +185,7 @@ private static String normalizePath(String path) {
Author author,
CommitMessageDto commitMessage,
@RequestConverter(ChangesRequestConverter.class) Iterable<Change<?>> changes) {
checkMirrorLocalRepo(repository, changes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we also need to deal with Thrift API?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's time to remove Thrift API? 😆
Thanks let me add this there too.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's time to remove Thrift API? 😆

🙈

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! @minwoox

@minwoox minwoox merged commit b6749e8 into line:master Aug 11, 2021
7 of 8 checks passed
@minwoox minwoox deleted the mirror_localRepo branch August 11, 2021 07:24
@minwoox
Copy link
Member Author

minwoox commented Aug 11, 2021

Thanks for reviewing. 😉

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

Successfully merging this pull request may close these issues.

None yet

2 participants