Authy push authentication feature #164

krzole · 2020-05-10T17:41:05Z

This pull request adds push authentication using Authy (https://authy.com/). If Authy authentication fails, continue with the classic Google OTP. See README.authy.md how to install and use.

googlebot · 2020-05-10T17:41:08Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

krzole · 2020-05-10T17:43:56Z

@googlebot I signed it!

googlebot · 2020-05-10T17:44:00Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

ThomasHabets · 2020-05-10T17:51:15Z

Oooh, very cool. Things this needs before being mergable:

This new functionality, because it depends on two new libraries, needs to be optional.
The README needs to reflect that it's not a fork. Probably the main README can point to the authy README since there's a fair difference.
The email address I think needs to match, like googlebot said.

I won't have time look at the code today. but looks very promising.

krzole · 2020-05-10T18:23:45Z

@googlebot I fixed it.

googlebot · 2020-05-10T18:23:50Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

krzole · 2020-05-11T14:13:37Z

@ThomasHabets: pull request is updated with the suggested changes

ThomasHabets · 2020-05-11T14:59:32Z

configure.ac

@@ -71,6 +71,38 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[

 AC_LANG_POP(C)

+AC_ARG_ENABLE([authy],
+	[AC_HELP_STRING([--enable-authy],


Why not enable it by default if the library is found?

Agreed, I'm going to rewrite configure.ac

ThomasHabets · 2020-05-11T15:06:41Z

configure.ac

+	[xenable_authy=no])
+AM_CONDITIONAL([ENABLE_AUTHY], [false])
+
+AC_CHECK_LIB([curl], [curl_global_init],


I'm not an autoconf expert, but why not just AC_CHECK_LIB([curl], [curl_global_init]) ?

Agreed, I'm going to rewrite configure.ac

ThomasHabets · 2020-05-11T15:07:46Z

src/pam_authy.c

+
+	curl = curl_easy_init();
+	if (!curl) {
+		log_message(LOG_ERR, pamh, "authy_err: curl init failed\n");


no need for newline.
Applies to all calls to log_message

ThomasHabets · 2020-05-11T15:08:25Z

src/pam_authy.c

+
+	asprintf(&url, "https://api.authy.com/onetouch/json/approval_requests/%s",
+			uuid);
+	asprintf(&xheader, "X-Authy-API-Key: %s", api_key);


asprintf is a GNU extension, so I think this might reduce the portability.

Agree, asprintfs will be removed

ThomasHabets · 2020-05-11T15:08:39Z

src/pam_authy.c

+
+	res = curl_easy_perform(curl);
+	if (res != CURLE_OK) {
+		log_message(LOG_ERR, pamh, "authy_err: curl call failed: %d (%s)\n", 


Remove newline

ThomasHabets · 2020-05-11T15:16:50Z

src/pam_authy.c

+
+static authy_rc_t authy_check_aproval(pam_handle_t *pamh, char *api_key, char *uuid)
+{
+	CURL *curl = NULL;


Define variables later. E.g. instead of what's here now, do:

CURL *curl = curl_easy_init();

That way the scope of variables is reduced making it clearer where they are set.

Agree, scope will be reduced

ThomasHabets · 2020-05-11T15:18:41Z

src/pam_authy.c

+		free(jt);
+
+	if (str)
+		free(str);


Return value of json_string_value must not be freed

ThomasHabets · 2020-05-11T15:20:52Z

src/pam_authy.c

+	}
+
+      log_message(LOG_INFO, pamh, "authy_dbg: Waiting for Authy authentication approval\n");
+	start_time = time(NULL);


Use clock_gettime() instead, so that this doesn't break if login happens when clock is set.

https://blog.habets.se/2010/09/gettimeofday-should-never-be-used-to-measure-time.html

ThomasHabets · 2020-05-11T15:22:48Z

src/pam_authy.c

+	asprintf(&data, "message=Login authentication");
+	asprintf(&data, "%s&details=%s at %s", data, username, hostname);
+	asprintf(&data, "%s&seconds_to_expire=%d", data, timeout);
+	curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, strlen(data));


Is there a client-side timeout in case the server hangs?

Agree, CURLOPT_TIMEOUT will be added to the curl call

ThomasHabets · 2020-05-11T15:24:45Z

src/pam_google_authenticator.c

+        authy_timeout = 30;
+      }
+      char *api_key = (char *)get_cfg_value_char(pamh, buf, "AUTHY_API_KEY");
+      arc = authy_login(pamh, authy_id, api_key, authy_timeout);


Have as much code as possible be not-ifdefed.

How about this: create a second file called pam_no_authy.c with an authy_login() implementation that always fails, logging that module was not built with authy support. That way no added ifdef in code, only in the Makefile.

I wanted to minimize the changes introduced to the original code.

I see your point, I'll create a dummy authy_login() function.

pam_no_authy.c contains a dummy authy_login() implementation that always fails. It is linked, when Authy support is disabled, limiting the number of 'ifdefs' in the pam_google_authenticator.c code.

krzole · 2020-05-20T10:02:17Z

@ThomasHabets Hello Thomas, how shall we continue with this pull request?

ThomasHabets · 2020-05-20T10:11:18Z

@krzole I've not had time to look at this more yet.

Although now I'm wondering if it actually belongs in the same module. How is this better than having a separate PAM module that does authy? The fallback logic is normally done in the PAM config.

krzole added 3 commits May 10, 2020 18:12

add authy authentication

355f40b

authy: move authy calls to pam_authy

eef6d6f

add README.authy.md

2b733f3

googlebot added the cla: no label May 10, 2020

ThomasHabets added the enhancement label May 10, 2020

googlebot added cla: yes and removed cla: no labels May 10, 2020

krzole added 2 commits May 11, 2020 13:49

authy: make authy feature optional (disabled by default)

f7459d2

authy: update README.authy.md

c27a6a5

ThomasHabets requested changes May 11, 2020

View reviewed changes

krzole added 12 commits May 16, 2020 00:47

authy: remove newlines from log_message() calls in pam_authy.c

90562d3

authy: remove asprintf() calls

a7c61a1

authy: dont check for NULL pointers before freeing them

f39faed

authy: check for pam_get_user() failure

09989ab

authy: fix typo in the function name

e17765d

authy: reduce variable scope

c054610

authy: use clock_gettime() to check the timeout

013618e

authy: fix too small string sizes

82941d0

authy: auto-enable authy support if dependencies are met

89ddccb

authy: add client-side timeouts with CURLOPT_TIMEOUT option

60ffea7

authy: add a dummy pam_no_authy.c file

ebfb63c

pam_no_authy.c contains a dummy authy_login() implementation that always fails. It is linked, when Authy support is disabled, limiting the number of 'ifdefs' in the pam_google_authenticator.c code.

authy: add licence header

7adaa1a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authy push authentication feature #164

Authy push authentication feature #164

krzole commented May 10, 2020

googlebot commented May 10, 2020

krzole commented May 10, 2020

googlebot commented May 10, 2020

ThomasHabets commented May 10, 2020

krzole commented May 10, 2020

googlebot commented May 10, 2020

krzole commented May 11, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

ThomasHabets May 11, 2020

krzole May 15, 2020

krzole commented May 20, 2020

ThomasHabets commented May 20, 2020

Authy push authentication feature #164

Are you sure you want to change the base?

Authy push authentication feature #164

Conversation

krzole commented May 10, 2020

googlebot commented May 10, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

krzole commented May 10, 2020

googlebot commented May 10, 2020

ThomasHabets commented May 10, 2020

krzole commented May 10, 2020

googlebot commented May 10, 2020

krzole commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzole commented May 20, 2020

ThomasHabets commented May 20, 2020