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.

Sign up for GitHub

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

Jump to bottom

securesystemslib: Add AnyNonemptyString, use for PATH_SCHEMA #244

Merged
merged 8 commits into from
Jun 22, 2020
Merged

securesystemslib: Add AnyNonemptyString, use for PATH_SCHEMA #244

merged 8 commits into from
Jun 22, 2020

Conversation

woodruffw
Copy link
Contributor

@woodruffw woodruffw commented Jun 18, 2020 •
edited
Loading

TODO: Needs tests.

Fixes issue #: #241.

Description of the changes being introduced by the pull request:

Please verify and check that the pull request fulfils the following
requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@woodruffw woodruffw mentioned this pull request Jun 18, 2020
Closed
@coveralls
Copy link

coveralls commented Jun 18, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 98.943% when pulling a3d0f58 on trailofbits:ww/any-nonempty-string into 9b3a78e on secure-systems-lab:master.

@woodruffw
Copy link
Contributor Author

test_B3_file_in_confined_directories currently fails on this:

self.assertTrue(in_confined_directory('a/b/c.txt', ['']))

...presumably because that second parameter is now being enforced via PATH_SCHEMA as a nonempty string. My intuition is that this should really become assertFalse, but that's a behavioral change. Thoughts @lukpueh @joshuagl @mnm678 @trishankatdatadog?

@lukpueh
Copy link
Member

lukpueh commented Jun 19, 2020

...presumably because that second parameter is now being enforced via PATH_SCHEMA as a non-empty string. My intuition is that this should really become assertFalse, but that's a behavioral change. Thoughts @lukpueh @joshuagl @mnm678 @trishankatdatadog?

I see this function for the first time. According to a comment, an empty string in the second parameter acts as some sort of "arbitrarily chosen" wildcard:

securesystemslib/securesystemslib/util.py

Lines 216 to 217 in 9b3a78e

# The empty string (arbitrarily chosen) signifies the client is confined
# to all directories and subdirectories. No need to check 'filepath'.

The only place where this function is used is in tuf.mirrors.get_list_of_mirrors, which btw. I'm also looking at for the first time. @trishankdatdatadog, do you know more about this? Confined directories are nowhere mentioned in the specification. Do we really need this function as securesystemslib.util? To me it looks like something that could be an inline check in get_list_of_mirrors.

@willwoodruff, for the time being I suggest to just patch that function to not validate the second argument as list of paths but rather as list of any strings (including empty string).

lukpueh
lukpueh reviewed Jun 19, 2020
View reviewed changes
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, @woodruffw! Do you have bandwidth to add tests?

securesystemslib/schema.py Outdated
def check_match(self, object):
if not isinstance(object, six.string_types):
raise securesystemslib.exceptions.FormatError('Expected a string'
' but got ' + repr(object))
Copy link
Member

Choose a reason for hiding this comment

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

Minor suggestion: We could subclass from AnyString and replace this first check with super().check_match.
Subclassing would also make it safe to remove the constructor here. Because the implicitly called AnyString.__init__(): pass will then prevent us from calling any further upper-level constructors (... which for some reason seem to be desired, or otherwise I don't understand the many __init__(): pass here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! super() is a bit messy since this code (appears to be) Python 2.7 compatible, so I called AnyString.check_match(self, object) explicitly. We could use super if we turned the top-level Schema class into a "new-style" one, but I figured that this was less invasive for a small change.

@woodruffw
Copy link
Contributor Author

for the time being I suggest to just patch that function to not validate the second argument as list of paths but rather as list of any strings (including empty string).

Sounds good to me. JFYI, you tagged another Will Woodruff 😉

I should also have some bandwidth for unit tests today.

@trishankatdatadog
Copy link
Contributor

Confined directories are nowhere mentioned in the specification. Do we really need this function as securesystemslib.util?

Yeah, this is some non-standard optional thing we should really get rid off. That, and the list of mirrors. Nobody uses them AFAIK. I would wholeheartedly endorse and merge any PR that does this.

lukpueh
lukpueh approved these changes Jun 22, 2020
View reviewed changes
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

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

[Apologies for mixing you two up, @willwoodruff and @woodruffw]

LGTM! Merging.

@lukpueh lukpueh merged commit df57705 into secure-systems-lab:master Jun 22, 2020
@lukpueh lukpueh mentioned this pull request Jun 22, 2020
Closed
@lukpueh
Copy link
Member

lukpueh commented Jun 22, 2020

Confined directories are nowhere mentioned in the specification. Do we really need this function as securesystemslib.util?

Yeah, this is some non-standard optional thing we should really get rid off. That, and the list of mirrors. Nobody uses them AFAIK. I would wholeheartedly endorse and merge any PR that does this.

Thanks for weighing in, @trishankatdatadog. I opened an issue: #245

@woodruffw woodruffw deleted the ww/any-nonempty-string branch June 22, 2020 17:10
@joshuagl joshuagl mentioned this pull request Jun 26, 2020
Closed
@lukpueh lukpueh mentioned this pull request Jun 30, 2020
Closed
3 tasks
MVrachev added a commit to MVrachev/tuf that referenced this pull request Jun 30, 2020
@MVrachev
Fix failing unit tests
4933f60 
Corresponding to the securesyslib changes made in this pr:
secure-systems-lab/securesystemslib#244
which changed the securesyslib.formats.PATH schema to be of type
AnyNonemptystring.
This made the tuf unit tests to fail because there are to places
where functional arguments should comfront to the
securesyslib.formats.PATH schema, but have a default value of
empty string.

Signed-off-by: Martin Vrachev <[email protected]>
MVrachev added a commit to MVrachev/tuf that referenced this pull request Jun 30, 2020
@MVrachev
Fix failing unit tests
0f9ccfc 
Corresponding to the securesyslib changes made in this pr:
secure-systems-lab/securesystemslib#244
which changed the securesyslib.formats.PATH schema to be of type
AnyNonemptystring.
This made the tuf unit tests to fail because there are to places
where functional arguments should comply to the
securesyslib.formats.PATH schema, but have a default value of
empty string.

Signed-off-by: Martin Vrachev <[email protected]>
MVrachev added a commit to MVrachev/tuf that referenced this pull request Jun 30, 2020
@MVrachev
Fix failing unit tests
853cdd3 
Corresponding to the securesyslib changes made in this pr:
secure-systems-lab/securesystemslib#244
which changed the securesyslib.formats.PATH schema to be of type
AnyNonemptystring.
This made the tuf unit tests to fail because there are to places
where functional arguments should comply with the
securesyslib.formats.PATH schema, but have a default value of
an empty string.

Signed-off-by: Martin Vrachev <[email protected]>
@MVrachev MVrachev mentioned this pull request Jun 30, 2020
Merged
3 tasks
sechkova pushed a commit to sechkova/tuf that referenced this pull request Jul 30, 2020
@MVrachev @sechkova
Fix failing unit tests
d2b138d 
Corresponding to the securesyslib changes made in this pr:
secure-systems-lab/securesystemslib#244
which changed the securesyslib.formats.PATH schema to be of type
AnyNonemptystring.
This made the tuf unit tests to fail because there are to places
where functional arguments should comply with the
securesyslib.formats.PATH schema, but have a default value of
an empty string.

Signed-off-by: Martin Vrachev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukpueh lukpueh lukpueh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@woodruffw @coveralls @lukpueh @trishankatdatadog