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

Added validator check for Mac OS private paths #309 #322

Merged
merged 7 commits into from Mar 5, 2019

Conversation

Projects
None yet
3 participants
@joachimmetz
Copy link
Member

joachimmetz commented Mar 1, 2019

No description provided.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 1, 2019

Codecov Report

Merging #322 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #322   +/-   ##
=======================================
  Coverage   91.19%   91.19%           
=======================================
  Files           7        7           
  Lines         420      420           
=======================================
  Hits          383      383           
  Misses         37       37

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 ec1c547...41c3bf5. Read the comment docs.

@joachimmetz joachimmetz requested a review from pstirparo Mar 1, 2019

@joachimmetz

This comment has been minimized.

Copy link
Member Author

joachimmetz commented Mar 1, 2019

@pstirparo could you spot check this PR and see if this is inline with our conversation about private paths on MacOS.

@joachimmetz joachimmetz self-assigned this Mar 1, 2019

@cugu

This comment has been minimized.

Copy link
Contributor

cugu commented Mar 1, 2019

Sorry for the interference, but what about

- type: FILE
  attributes: {paths: ['/private/etc/ssh/sshd_config']}
  supported_os: [Darwin]
- type: FILE
  attributes: {paths: ['/etc/ssh/sshd_config']}
  supported_os: [Darwin,Linux]

instead of

- type: FILE
  attributes:
    paths:
    - '/etc/ssh/sshd_config'
    - '/private/etc/ssh/sshd_config'
  supported_os: [Darwin]
- type: FILE
  attributes: {paths: ['/etc/ssh/sshd_config']}
  supported_os: [Linux]
@joachimmetz

This comment has been minimized.

Copy link
Member Author

joachimmetz commented Mar 1, 2019

Sorry for the interference, but what about

Interesting idea but IMHO a couple of downsides of that approach:

  • slightly more complex conditional logic is necessary to validate
  • easy to overlook the second source as one that applied to Darwin

For now I opt to keep it as is, and revisit this idea when I address the supported_os #274

@cugu

This comment has been minimized.

Copy link
Contributor

cugu commented Mar 1, 2019

Validation of duplicate paths #41 is more complicated this way.

@joachimmetz

This comment has been minimized.

Copy link
Member Author

joachimmetz commented Mar 1, 2019

Validation of duplicate paths #41 is more complicated this way.

could you explain/elaborate why you think this is the case?

@cugu

This comment has been minimized.

Copy link
Contributor

cugu commented Mar 1, 2019

With the current solution there are duplicates of file paths when ignoring the supported os. My solution prevents duplicate file paths.

@joachimmetz

This comment has been minimized.

Copy link
Member Author

joachimmetz commented Mar 1, 2019

My solution prevents duplicate file paths.

How? I'm missing the necessary context.

Your suggested approach reduces the need for duplication in a single artifact definition, but does not prevent it.

However duplication can still exist across artifact definitions, which is what hinted at in #41.

@cugu

This comment has been minimized.

Copy link
Contributor

cugu commented Mar 1, 2019

A simple validator for duplicate file paths will find the duplicate /etc/ssh/sshd_config in

- type: FILE
  attributes:
    paths:
    - '/etc/ssh/sshd_config'
    - '/private/etc/ssh/sshd_config'
  supported_os: [Darwin]
- type: FILE
  attributes: {paths: ['/etc/ssh/sshd_config']}
  supported_os: [Linux]

but not in

- type: FILE
  attributes: {paths: ['/private/etc/ssh/sshd_config']}
  supported_os: [Darwin]
- type: FILE
  attributes: {paths: ['/etc/ssh/sshd_config']}
  supported_os: [Darwin,Linux]

Anyway it does not matter to much, I just have to make my duplication checker aware of different supported_os.

@joachimmetz

This comment has been minimized.

Copy link
Member Author

joachimmetz commented Mar 1, 2019

Anyway it does not matter to much, I just have to make my duplication checker aware of different supported_os.

Note that supported_os is on the list to be changed, I opt not to invest too much effort here yet.

@pstirparo
Copy link
Contributor

pstirparo left a comment

LGTM

@joachimmetz joachimmetz merged commit d5bf9a9 into ForensicArtifacts:master Mar 5, 2019

5 checks passed

CodeFactor No issues found.
Details
codecov/patch Coverage not affected when comparing ec1c547...41c3bf5
Details
codecov/project 91.19% remains the same compared to ec1c547
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joachimmetz joachimmetz deleted the joachimmetz:private branch Mar 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.