Page MenuHomePhabricator

[services] Backup - authentication handlers
AbandonedPublic

Authored by karol on Feb 18 2022, 5:01 AM.
Tags
None
Referenced Files
F3527759: D3245.id9835.diff
Tue, Dec 24, 6:34 AM
Unknown Object (File)
Fri, Dec 20, 5:04 PM
Unknown Object (File)
Fri, Dec 20, 5:04 PM
Unknown Object (File)
Fri, Dec 20, 5:04 PM
Unknown Object (File)
Fri, Dec 20, 5:03 PM
Unknown Object (File)
Fri, Dec 20, 5:03 PM
Unknown Object (File)
Fri, Dec 20, 5:03 PM
Unknown Object (File)
Fri, Dec 20, 5:03 PM

Details

Summary

This is a basic structure for the authentication handlers. Authentication is not yet fully figured out therefore this is more like a draft but I wanted to prepare a clear structure for how we may want to handle this.

Depends on D3200

Test Plan
cd services
yarn run-backup-service

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, max, varun, jim.
tomek requested changes to this revision.Feb 22 2022, 3:47 AM
tomek added inline comments.
services/backup/docker-server/contents/server/src/Authentication/AuthenticationHandlerBase.h
7 ↗(On Diff #9801)

I think that this namespace is confusing. Is it referring cryptography, cryptocurrency, or something else... what do you think about changing this name to auth?

11–15 ↗(On Diff #9801)

This comment explains something, but to be really helpful it needs to be extended. Please explain what is the purpose of this function and use full sentences (starting with uppercase letter, ending with period) instead of sequences of words.

once an empty string is returned we should verify the state

After some time this info makes sense, but it is inconsistent with the first part and that makes it more confusing. The first part of this comment refers to the function in third person, so we need to be consistent here and write something like: When this function returns an empty string, we should....
And the last part: who are we. Please be more specific here. Is it a client? Or is it a process in the service? Or something completely different.

Overall we should try to write comments so that a reader does not need to have deep knowledge to understand them.

By the way, what are the thread-safety guarantees an implementation should provide? Are we using this code from multiple threads, or do we expect to have thread_local instances? It would really help if either a summary or this comment explained how this class should be used (if we want to have a single instance, a thread_local instance or a dedicated instance for every auth session). My guess is that the third option would be the best as it will make state management the least complicated.

services/backup/docker-server/contents/server/src/Authentication/AuthenticationManager.cpp
9 ↗(On Diff #9801)

Is this manager used for a single connection? Or do we expect it to handle multiple connections?

13–27 ↗(On Diff #9801)

My intuition is that we can take a different approach to make this more maintainable and readable. So when we start auth process we should determine if we want to use Pake or wallet, then create a concrete auth manager, store it as an instance variable and forward all the traffic to it. By doing that we would avoid having method-specific code in AuthenticationManager which would make the code simpler and more open to extension.

services/backup/docker-server/contents/server/src/Authentication/AuthenticationManager.h
22–23 ↗(On Diff #9801)

I suggested a different approach in one comment (you can find advantages there). Instead of having two different authentication manager, you could have only one, specific and forward all the calls to it.

This revision now requires changes to proceed.Feb 22 2022, 3:47 AM

namespace change crypto->auth

services/backup/docker-server/contents/server/src/Authentication/AuthenticationHandlerBase.h
7 ↗(On Diff #9801)

I was thinking of cryptography, we can do auth, it seems better.

services/backup/docker-server/contents/server/src/Authentication/AuthenticationManager.cpp
9 ↗(On Diff #9801)

one manager per connection

services/backup/docker-server/contents/server/src/Authentication/AuthenticationHandlerBase.h
11–15 ↗(On Diff #9801)

This was more like a high-level concept of how it's gonna work. I'm going to try to make it more clear.

By the way ...

This code's going to be used from multiple threads but not at the same time. The way grpc works with the async API is that it takes a thread from some thread pool and uses it to process the next request. However, for every connection there will be a separate auth manager object that will contain an auth handler object as well, so effectively there will be a separate auth handler for every connection as well.
Even though there may be different threads accessing the handler's resources, the requests will be handled sequentially (one after another).
So since there's not going to be a situation when multiple threads try to access the same resources, we should not worry about thread-safety, right?

services/backup/docker-server/contents/server/src/Authentication/AuthenticationManager.cpp
13–27 ↗(On Diff #9801)

good idea, thanks

tomek requested changes to this revision.Mar 8 2022, 7:24 AM

Haven't reviewed this in depth now, as we first should clarify multithreading issues

services/backup/docker-server/contents/server/src/Authentication/AuthenticationHandlerBase.h
11–15 ↗(On Diff #9801)

I don't think so. Even when two threads don't access a resource at the same time, we need some synchronization to let one thread know the current state of that resource. The simplest scenario which demonstrates the issue is when one thread modifies a resource in CPU register which isn't saved to memory until necessary. When the second thread accesses this resource, it is read from memory where an outdated value is stored. The only way of introducing "happens before" relationship is by synchronization, which can be achieved by e.g. using atomic values.

This revision now requires changes to proceed.Mar 8 2022, 7:24 AM
services/backup/docker-server/contents/server/src/Authentication/AuthenticationHandlerBase.h
11–15 ↗(On Diff #9801)

Discussed offline. We indeed need to use some synchronization here. A little explanation: https://stackoverflow.com/questions/22798873/visibility-in-concurrent-c-programs

return pointer from processRequest

tomek requested changes to this revision.Mar 16 2022, 9:36 AM

It looks great with all this refactoring and splitting. Requesting changes, as I have some questions, but overall it's really nice!

services/backup/docker-server/contents/server/src/Authentication/AuthenticationHandlerBase.h
23

Can we change this link to https://en.cppreference.com/w/cpp/language/memory_model#Threads_and_data_races? The C++ reference seems like a better source

services/backup/docker-server/contents/server/src/Authentication/AuthenticationManager.cpp
16

Does grpc enforce this formatting?

services/backup/docker-server/contents/server/src/Authentication/AuthenticationManager.h
22

Shouldn't we also make this atomic?

services/backup/docker-server/contents/server/src/Authentication/PakeAuthenticationHandler.cpp
7–8

Shouldn't the authenticationType always be PAKE? I guess it doesn't make much sense to have this as a constructor argument - we should always return PAKE in getAuthenticationType, right?

This revision now requires changes to proceed.Mar 16 2022, 9:36 AM

Abandoning this since we're going to handle authentication in a separate service.