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

Adding BoringSSL runner. #49

Merged
merged 5 commits into from
Apr 26, 2021
Merged

Adding BoringSSL runner. #49

merged 5 commits into from
Apr 26, 2021

Conversation

xvzcf
Copy link
Owner

@xvzcf xvzcf commented Apr 21, 2021

No description provided.

@chris-wood
Copy link
Collaborator

@xvzcf is there a reason why we can't get the tool files from the cloned version of BoringSSL, rather than bake them in here?

@xvzcf
Copy link
Owner Author

xvzcf commented Apr 22, 2021

The files here are the tool sources with a decent amount of code removed and some modifications made (the runner returns 64 when a testcase is unsupported for example).

The reason for doing this was to get fine grained control over the setup, teardown, and validation for each testcase; on the client side dc testcase for example, this check is run after the connection is completed.

As we start coming up with more complicated testcases, it seems like the flexibility we get from calling library code directly would make things easier as opposed to having to conform to/extend the tool command line API.

@dmcardle
Copy link

The goal is to use only public APIs, right? I think this is the right way to do it. BoringSSL's client/server command-line flags have no stability guarantee, so it makes sense to maintain your own.

@xvzcf xvzcf marked this pull request as ready for review April 22, 2021 16:10
@xvzcf xvzcf requested a review from chris-wood April 22, 2021 17:16
@chris-wood
Copy link
Collaborator

This LGTM then 👍 When BoringSSL adds ECH support for draft-10, we can update the runner to handle that test case, too.

@xvzcf
Copy link
Owner Author

xvzcf commented Apr 22, 2021

Sounds good.

I also think there's also a broader discussion here: currently the Dockerfile pulls my personal branch which has the latest DC spec implementation, but for ECH we'll be using master. To get both DC and ECH support, do I rebase my branch off the BoringSSL master when ECH draft-10 goes in? Do I build binaries from both branches and build two runners? Or do I create two Dockerfiles representing two slightly different BoringSSL endpoints (one based on my branch and one based on master) and modify the cmd/runner code accordingly?

I'm leaning towards some sort of separation, since the code in master is more thoroughly reviewed and tested than the one in my branch, but I'm not yet sure what the cleanest way of going about this would be.

@xvzcf xvzcf merged commit ac33b47 into main Apr 26, 2021
@xvzcf xvzcf deleted the bssl-runner branch April 26, 2021 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants