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

Unauthenticated handlers #962

Merged
merged 21 commits into from
Mar 19, 2020
Merged

Conversation

jumaffre
Copy link
Contributor

Building block for #926

This PR introduces a new caller_auth_disabled handler flag. If the flag is set (default to false), the handler does not go through the usual caller validation process as anyone can execute the corresponding endpoint.

As well as unit tests (see frontend_test.cpp), I've added a new endpoint to the logging app that lets anonymous users (and only anonymous users) register "anonymous" messages.

Most of the changes are in frontend.h:

  • A lot of the code was moved from process() to process_command() as the handler now influences the authorisation policy.
  • Similarly, some of the forwarding logic was moved down to process_command().

@jumaffre jumaffre requested a review from a team as a code owner March 18, 2020 14:14
src/enclave/rpccontext.h Outdated Show resolved Hide resolved
@codecov-io
Copy link

Codecov Report

Merging #962 into master will decrease coverage by 0.06%.
The diff coverage is 77.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #962      +/-   ##
==========================================
- Coverage   68.09%   68.03%   -0.06%     
==========================================
  Files         103      103              
  Lines        8147     8145       -2     
==========================================
- Hits         5547     5541       -6     
- Misses       2600     2604       +4     
Flag Coverage Δ
#unit_BFT 68.03% <77.19%> (-0.06%) ⬇️
#unit_CFT 68.02% <77.19%> (-0.06%) ⬇️
Impacted Files Coverage Δ
src/enclave/rpccontext.h 85.71% <ø> (ø)
src/node/rpc/handlerregistry.h 78.95% <66.67%> (-4.03%) ⬇️
src/node/rpc/frontend.h 63.43% <80.00%> (-1.34%) ⬇️
src/node/rpc/memberfrontend.h 67.47% <100.00%> (+0.06%) ⬆️
src/tls/keypair.h 74.38% <0.00%> (-0.31%) ⬇️

@ghost
Copy link

ghost commented Mar 18, 2020

unauthenticated_handlers@6093 aka 20200319.4 vs master ewma over 30 builds from 5804 to 6087
images

src/node/rpc/frontend.h Outdated Show resolved Hide resolved
tests/e2e_logging.py Outdated Show resolved Hide resolved
src/node/rpc/frontend.h Outdated Show resolved Hide resolved
@@ -92,6 +93,24 @@ namespace ccf
return *this;
}

// If true, caller does not need to be authenticated
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like exactly the same thing as require_client_signature. The comments around its use-point also talk about requiring auth. Convince me these are different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The two flags are different. require_client_signature indicates that the RPC requires a signed command (i.e. use scurl.sh) while caller_auth_disabled indicates that the client does not need to be authenticated (i.e. not specifying --key and --cert to curl is OK), even on a frontend that has (through its registry) a certs table. In practice however, I imagine that the two are not completely orthogonal (e.g. using proxies).

Copy link
Member

Choose a reason for hiding this comment

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

Both could be considered authentication, if we had a mechanism to convey intended identity on signed messages?

@jumaffre
Copy link
Contributor Author

PBFT performance drop with client signatures is expected as we now verify the client signature on each node before reaching consensus as opposed to primary node only.

@jumaffre jumaffre merged commit f534a89 into microsoft:master Mar 19, 2020
eddyashton pushed a commit to eddyashton/CCF that referenced this pull request Mar 24, 2020
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.

5 participants