Cert based authentication #116
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dorileo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
dorileo
requested review from
ericdand,
illfelder,
Quintonamore and
ChaitanyaKulkarni28
| @@ -23,10 +23,6 @@ | |||
| #include <sys/syslog.h> | |||
| #include <sys/types.h> | |||
|
|
|||
| #ifdef __cplusplus | |||
There was a problem hiding this comment.
why did this exist before? was there a distro that depended on it?
There was a problem hiding this comment.
basically it was a: "I wrote it in C" rather then C++, no actual reason.
dorileo
force-pushed
the
cert-based-authentication
branch
3 times, most recently
from
10aa2b4
to
36a5593
Compare
| fp_len = FingerPrintFromBlob(cert, &fingerprint); | ||
| if (fp_len == 0) { | ||
| SysLogErr("Could not extract/parse fingerprint from certificate."); | ||
| goto fail; |
There was a problem hiding this comment.
I think you may want to free(fingerprint) here too, though I suppose the program will terminate soon and get cleaned up anyway, so a memory leak doesn't matter too much here.
There was a problem hiding this comment.
for sure, we should have a free() in the fail block. I'll get that updated. Thanks.
There was a problem hiding this comment.
oops, I just re-reviewed the code, it actually shouldn't free(fingerprint); in the fail block, all the error conditions for fail: happen either before having a fingerprint allocated or by failing to allocate it.
| return false; | ||
| } | ||
|
|
||
| if (!ParseJsonToSuccess(response)) { |
There was a problem hiding this comment.
Small nit: You name this method ParseJson... but it seems to return a true or false value indicating authorization. I would recommend renaming it to clearly express what it actually does (i.e. not only parsing).
There was a problem hiding this comment.
I can do it, In a follow up PR maybe, I'm using/reusing the existing function.
| } | ||
|
|
||
| users_filename = kUsersDir; | ||
| users_filename.append(user_name); |
There was a problem hiding this comment.
C question: won't this append the user_name to the end of the kUsersDir char array? Or does the previous line (line 1338) cause the program to copy the string quietly? If this program ever runs twice without exiting, will this cause unexpected malformed file paths?
There was a problem hiding this comment.
It's more a C++ thing, line 1338 is creating an instance of string copying the the char array (kUserDir).
There was a problem hiding this comment.
Nice, glad that's the case. Thanks for humouring me. I'm not a C++ expert by any means so it's not always clear to me whether implicit conversions like this create copies or just pointers. ;)
dorileo
force-pushed
the
cert-based-authentication
branch
from
36a5593
to
413886b
Compare
Introduce a oslogin_sshca namespace, remove the C de mangling extern.
With this patch now we can have a global sys logger having the logging points present whether the sys logger has been setup/initialized or not. For unit tests for example we'll not have it initialized rendering into no-op calls to SysLogErr().
The AuthorizeUser() API merges together the authorization operations for both login and adminLogin authorize policies. This API is meant to be used in single points of Authorization - where both login & adminLogin are attempted/processed.
As we are moving authorization out of pam modules it makes sense to have oslogin_sshca.o in the root dir of src side-by-side with oslogin_utils.o.
Start disaging the use of oslogin_sshca in the pam modules.
In a model using AuthorizedPrincipalsCommand we can handle a ssh cert only - not having to split and ignore method and algorithm tokens.
Update both authorized_keys and authorized_keys_sk to use new sys logger facilities as well as AuthorizeUser().
pam_oslogin_admin is not required anymore and pam_oslogin_login is now only responsible to handle 2fa.
dorileo
force-pushed
the
cert-based-authentication
branch
from
413886b
to
5a74915
Compare
|
/lgtm |
|
Just sent a PR updating the OSLogin CIT tests. |
|
/unhold |
google-oss-prow
bot
merged commit 1aebe41
into
GoogleCloudPlatform:master
No description provided.