Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
snap/validate: extend socket validation tests #4219
Conversation
jdstrand
requested changes
Nov 15, 2017
Thanks for this PR! One small change and please see #3916 (comment)
| @@ -253,6 +253,9 @@ func (s *ValidateSuite) TestValidateAppSocketsInvalidListenStreamPathRelative(c | ||
| func (s *ValidateSuite) TestValidateAppSocketsInvalidListenStreamAbstractSocket(c *C) { | ||
| app := createSampleApp() | ||
| invalidListenAddresses := []string{ | ||
| + "@snap.myapp", | ||
| + "@snap.myapp.foo", | ||
| + "@snap.myapp\\000.foo", |
jdstrand
Nov 15, 2017
Contributor
I was thinking that 'myapp' was the the snap name, but it is 'mysnap'. Can you remove '@snap.myapp.foo' since it is covered below, then change the other two to be:
"@snap.mysnap",
"@snap.mysnap\\000.foo",
albertodonato
added some commits
Nov 15, 2017
codecov-io
commented
Nov 15, 2017
•
Edited 4 times
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 16, 2017
-
codecov-io
Nov 15, 2017
-
Nov 16, 2017 -
Nov 16, 2017 -
Nov 16, 2017 -
Nov 15, 2017
Codecov Report
@@ Coverage Diff @@
## master #4219 +/- ##
==========================================
+ Coverage 75.86% 75.88% +0.02%
==========================================
Files 440 440
Lines 38247 38320 +73
==========================================
+ Hits 29016 29080 +64
- Misses 7218 7224 +6
- Partials 2013 2016 +3
Continue to review full report at Codecov.
|
jdstrand
requested changes
Nov 15, 2017
Thanks for the changes! What is here looks good.
I still would like to see a test for trying to assign a string like "abc" or "rw-rw-rw-" to SocketMode though (both should fail with my understanding of the design of the feature). The Layout code seems to be doing a decent job of this because it uses m, err := strconv.ParseUint(l.Mode, 8, 32) and returns an error if there is one (line 147 in snap/info_snap_yaml.go), but I don't see where we are doing that anywhere for SocketMode.
|
@jdstrand the code you pointed out is there because in the Layout those options' value is a string, so it needs to be converted. In the case of SocketMode, it's an os.FileMode, so a string would trigger a validation error from the json loader. I added a test for this case. |
jdstrand
approved these changes
Nov 15, 2017
Thanks! Since the yaml is untrusted input, I wanted to make sure we had tests that verified everything was ok in case of future code changes.
|
@albertodonato - I just realized that validateSocketAddrNetPort() is incomplete. Valid port range is 1-65535. It should be adjusted with tests for:
It also occurred to me that the socket validation PR #3916 doesn't seem to account for two snaps requesting the same port. What will systemd do if:
snapd actually has everything it needs to make sure there are no conflicts there. I suggest this be fixed up in a followup PR. @mvo5, what do you think? |
jdstrand
requested changes
Nov 15, 2017
See my last comment wrt port ranges for this PR. Port conflicts are a larger issue and should be handled separately.
|
@jdstrand added port range validation |
| + socket-mode: asdfasdf | ||
| +`) | ||
| + _, err := snap.InfoFromSnapYaml(y) | ||
| + c.Assert(err, NotNil) |
zyga
Nov 16, 2017
Contributor
Nitpick: use ErrorMatches to highlight what this test is really checking.
albertodonato
Nov 16, 2017
Contributor
I compared the whole message, since I couldn't get the regexp to work
| +// validateSocketmode checks that the socket mode is a valid file mode. | ||
| +func validateSocketMode(mode os.FileMode) error { | ||
| + if mode > 0777 { | ||
| + return fmt.Errorf("invalid socket mode: %04o", mode) |
| - return fmt.Errorf("socket %q has invalid %q port number %q", socket.Name, fieldName, port) | ||
| + var val uint64 | ||
| + var err error | ||
| + retErr := fmt.Errorf("socket %q has invalid %q port number %q", socket.Name, fieldName, port) |
zyga
Nov 16, 2017
Contributor
invalid %q port number %q -- what will be the value of the first %q (field name)?
albertodonato
Nov 16, 2017
Contributor
it's the name of the field under validation, specifically "listen-stream"
| + app := createSampleApp() | ||
| + app.Sockets["sock"].SocketMode = 1234 | ||
| + err := ValidateApp(app) | ||
| + c.Assert( |
albertodonato commentedNov 15, 2017
•
Edited 1 time
-
albertodonato
Nov 15, 2017
This addresses comments on #3916 (review) (PR was merged already)