Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Figure out how to make updating less painful #4

Open
davidben opened this issue Jun 27, 2016 · 1 comment
Open

Figure out how to make updating less painful #4

davidben opened this issue Jun 27, 2016 · 1 comment

Comments

@davidben
Copy link

Right now we have a large diff on bssl_shim and a small diff on runner.go. It'd be nice to avoid rebasing being a hassle. Filing this so we have a place to figure out ideas.

I think ideally we'd get rid of the runner.go diff. For the shim, I think we want to move to this repo containing a fork of bssl_shim rather than patches. It seems the diff is large enough that patches aren't quite reasonable? However, the churn might make that unrealistic. Not sure. There are probably changes to be made to cut down on churn in the shim.

Some ideas:

  • Move all the CheckHandshakeProperties logic out of the shim. The shim perhaps has an extra channel to the runner and spits out some kind of (key, value) list. Then the Go code can assert on all that.
  • Better story for unimplemented stuff. Test suppressions in config files? The command-line parser has a dedicated exit code for unknown flags that the runner interprets as a skip?
  • Figure out what to do about all the expectedError checks. We could either have a config file mapping to OpenSSL strings or perhaps we add the right expectedLocalError (runner-side) and explore what value expectedError (shim-side) gives us that expectedLocalError can't.
  • A few flags control "API" behavior that, in theory, should be invisible, like -async and -implicit-handshake. Different implementations may have different API modes they'd like to test. Perhaps those shouldn't be controlled by the test case and instead runner should just take a set of "configurations" and do the cross product?
    • Some tests might be sensitive to this for weird reasons. Would need to sort that out, or config should say "skip X test for Y config".
    • This would make tests take even longer to run, but maybe that's fine?
    • Another nuisance is some "modes" are probably specific to certain tests, like OpenSSL/BoringSSL have multiple client auth configuration APIs. Perhaps we should specify that some modes only apply to some tests?
  • There's flags that control the shim's read/write pattern like -write-different-record-sizes, -shim-writes-first, -shim-shuts-down. Those have always bugged me a little. It would be good if ossl_shim needn't update every time we need a new read/write pattern. Maybe there should be some "control" channel to the shim so we can just tell the shim "please read 5 bytes now and tell me what you think you got", "please write this data now", "please shutdown now", etc. That could be the same channel as the (key, value) results list, perhaps.

@ekasper @mattcaswell, what are your thoughts? Most of this would involve changes in BoringSSL, but I'm open to changing the BoringSSL copy where helpful. (Assuming, of course, they don't get in the way of satisfying our own testing needs. But most of these changes seem like general cleanliness improvements anyway. Test suppressions and stuff aren't as useful for us, but shouldn't be too burdensome.)

@davidben
Copy link
Author

Better story for unimplemented stuff. Test suppressions in config files? The command-line parser has a dedicated exit code for unknown flags that the runner interprets as a skip?

As of https://boringssl.googlesource.com/boringssl/+/842ae6cad06c0c80886021b8fc86b0ffc16ab59c, runner now interprets exit code 89 from the shim as unimplemented. If -allow-unimplemented is passed in, those tests will be silently skipped.

(Thanks, @ekr!)

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

No branches or pull requests

1 participant