Обсуждение: Change authentication error message (patch)
Hello,
Instead of pushing extra info to the logs I decided that we could
without giving away extra details per policy. I wrote the error message
in a way that tells the most obvious problems, without admitting to any
of them. Please see attached:
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 415b614..a775534 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -270,7 +270,7 @@ auth_failed(Port *port, int status) break; case uaPassword:
case uaMD5:
- errstr = gettext_noop("password authentication
failed for user \"%s\"");
+ errstr = gettext_noop("password, username or
password expiry failed for user \"%s\""); /* We use it to indicate if a .pgpass password
failed. */ errcode_return = ERRCODE_INVALID_PASSWORD; break;
On 06/16/2013 06:02 PM, Joshua D. Drake wrote: > Instead of pushing extra info to the logs I decided that we could > without giving away extra details per policy. I wrote the error message > in a way that tells the most obvious problems, without admitting to any > of them. Please see attached: +1 for solving this with a bit of word-smithing. However, the proposed wording doesn't sound like a full sentence to my ears, because a password or username cannot fail per-se. How about: "password authentication failed or account expired for user \"%s\"" It's a bit longer, but sounds more like a full sentence, no? Regards Markus Wanner
On 06/18/2013 02:25 AM, Markus Wanner wrote: > > On 06/16/2013 06:02 PM, Joshua D. Drake wrote: >> Instead of pushing extra info to the logs I decided that we could >> without giving away extra details per policy. I wrote the error message >> in a way that tells the most obvious problems, without admitting to any >> of them. Please see attached: > > +1 for solving this with a bit of word-smithing. > > However, the proposed wording doesn't sound like a full sentence to my > ears, because a password or username cannot fail per-se. I believe it actually can. The error message that is returned for a bad password, bad user or expired password is all the same. Which is why I put the username in there. > > How about: > "password authentication failed or account expired for user \"%s\"" > > It's a bit longer, but sounds more like a full sentence, no? Yes but I don't think it is accurate, what about: "Authentication failed or password has expired for user \"%s\"" Authentication failed covers any combination of a username/password being wrong and obviously password expired covers the other. Sincerely, Joshua D. Drake > > Regards > > Markus Wanner > -- Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats
This probably is nit-picking, but it interests me in terms of how the language is used and understood. On 06/19/2013 08:55 PM, Joshua D. Drake wrote: > I believe it actually can. The error message that is returned for a bad > password, bad user or expired password is all the same. Which is why I > put the username in there. Sure, the authentication can fail for all these reasons. What I stumbled over was the formulation of a "failed username". If an engine fails, it might literally fall apart. The username itself - even if it doesn't pass authentication - is not falling apart in the same sense. But does the username (or the password) fail if authentication with it (in combination with password and account expiration time) is not possible? After all, it might still a valid and complete username for another cluster or another service. You can probably say: "that username failed" when you actually mean it "failed to authenticate together with the provided password". Or how do English native speakers perceive this? > "Authentication failed or password has expired for user \"%s\"" > > Authentication failed covers any combination of a username/password > being wrong and obviously password expired covers the other. Works for me. Considering the password to be the thing that expires (rather than the account) is probably more accurate as well. Regards Markus Wanner
On 06/19/2013 01:18 PM, Markus Wanner wrote:
>> "Authentication failed or password has expired for user \"%s\""
>>
>> Authentication failed covers any combination of a username/password
>> being wrong and obviously password expired covers the other.
>
> Works for me. Considering the password to be the thing that expires
> (rather than the account) is probably more accurate as well.
It is also how it is worded in the docs (which is why I used it). Patch
below.
JD
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 415b614..f129fe1 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -270,7 +270,7 @@ auth_failed(Port *port, int status) break; case uaPassword:
case uaMD5:
- errstr = gettext_noop("password authentication
failed for user \"%s\"");
+ errstr = gettext_noop("Authentication failed or
password has expired for user \"%s\""); /* We use it to indicate if a .pgpass password
failed. */ errcode_return = ERRCODE_INVALID_PASSWORD; break;
--
Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc
For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats
On Wed, Jun 19, 2013 at 11:55 AM, Joshua D. Drake <jd@commandprompt.com> wrote:
On 06/18/2013 02:25 AM, Markus Wanner wrote:
On 06/16/2013 06:02 PM, Joshua D. Drake wrote:Yes but I don't think it is accurate, what about:
How about:
"password authentication failed or account expired for user \"%s\""
It's a bit longer, but sounds more like a full sentence, no?
"Authentication failed or password has expired for user \"%s\""
I think we need to keep the first "password". "Password authentication" is a single thing, it is the authentication method attempted. It is the password method (which includes MD5) which failed, as opposed to the LDAP method or the Peer method or one of the other methods.
Without this level of explicitness, it might be hard to figure out which row in pg_hba.conf was the one that PostgreSQL glommed onto to use for authentication. (Although by this argument, I don't know why MD5 doesn't get its own message specific to it, rather than sharing plain password)
Cheers,
Jeff
On 06/20/2013 12:51 AM, Jeff Janes wrote: > I think we need to keep the first "password". "Password authentication" > is a single thing, it is the authentication method attempted. It is the > password method (which includes MD5) which failed, as opposed to the > LDAP method or the Peer method or one of the other methods. That's against the rule of not revealing any more knowledge than a potential attacker already has, no? For that reason, I'd rather go with just "authentication failed". > Without this level of explicitness, it might be hard to figure out which > row in pg_hba.conf was the one that PostgreSQL glommed onto to use for > authentication. As argued before, that should go into the logs for diagnosis by the sysadmin, but should not be revealed to an attacker. Regards Markus Wanner
On 20/06/2013 08:47, Markus Wanner wrote:
> On 06/20/2013 12:51 AM, Jeff Janes wrote:
>> I think we need to keep the first "password". "Password authentication"
>> is a single thing, it is the authentication method attempted. It is the
>> password method (which includes MD5) which failed, as opposed to the
>> LDAP method or the Peer method or one of the other methods.
>
> That's against the rule of not revealing any more knowledge than a
> potential attacker already has, no? For that reason, I'd rather go with
> just "authentication failed".
My understanding is that the attacker would already have that
information since the server would have sent an
AuthenticationMD5Password message to get to the error in the first
place. And we still reveal the authentication method to the frontend in
all other cases ("peer authentication failed", for example).
>> Without this level of explicitness, it might be hard to figure out which
>> row in pg_hba.conf was the one that PostgreSQL glommed onto to use for
>> authentication.
>
> As argued before, that should go into the logs for diagnosis by the
> sysadmin, but should not be revealed to an attacker.
Isn't the point of this patch exactly that we didn't want to go down
that road? I.e. "password authentication failed" didn't say that the
password might've expired, but some people thought just logging a
WARNING/LOG wasn't enough.
Regards,
Marko Tiikkaja
On 06/20/2013 12:27 PM, Marko Tiikkaja wrote:
> My understanding is that the attacker would already have that
> information since the server would have sent an
> AuthenticationMD5Password message to get to the error in the first
> place. And we still reveal the authentication method to the frontend in
> all other cases ("peer authentication failed", for example).
Oh, right, I wasn't aware of that. Never mind, then.
+1 for keeping it mention "password authentication" explicitly.
However, thinking about this a bit more: Other authentication methods
may also provide password (or even account) expiration times. And may
fail to authenticate a user for entirely different reasons.
Given that, I wonder if "password expired" is such a special case worth
mentioning in case of the "password auth" method. If we go down that
path, don't we also have to include "auth server unreachable" as a
possible cause for authentication failure for methods that use an
external server?
Regards
Markus Wanner
On Wed, Jun 19, 2013 at 01:27:39PM -0700, Joshua D. Drake wrote:
>
> On 06/19/2013 01:18 PM, Markus Wanner wrote:
>
> >>"Authentication failed or password has expired for user \"%s\""
> >>
> >>Authentication failed covers any combination of a username/password
> >>being wrong and obviously password expired covers the other.
> >
> >Works for me. Considering the password to be the thing that expires
> >(rather than the account) is probably more accurate as well.
>
> It is also how it is worded in the docs (which is why I used it).
> Patch below.
I have developed the attached patch to fix this problem. Do I need to
say "invalid user or invalid or expired password"?
---------------
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Вложения
Bruce Momjian <bruce@momjian.us> writes:
> I have developed the attached patch to fix this problem. Do I need to
> say "invalid user or invalid or expired password"?
I'm not convinced that this improves anything. The problem might not in
fact be either of the things you mention, in which case the new message
is outright misleading. Also, what of the policy stated in the header
comment for the function you're hacking, ie we intentionally don't reveal
the precise cause of the failure to the client?
regards, tom lane
On Thu, Jan 23, 2014 at 10:39:34PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > I have developed the attached patch to fix this problem. Do I need to > > say "invalid user or invalid or expired password"? > > I'm not convinced that this improves anything. The problem might not in > fact be either of the things you mention, in which case the new message > is outright misleading. Also, what of the policy stated in the header > comment for the function you're hacking, ie we intentionally don't reveal > the precise cause of the failure to the client? Well, the only solution then would be to add some weasel words like "perhaps expired password", but that seems so rare that I doubt it would apply very often and seems like an odd suggestion. We could go with: password authentication failed for user \"%s\": perhaps invalid or expired password We did have two threads on this issue in the past 12 months so I figured we should try to do something. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Thu, Jan 23, 2014 at 10:39:34PM -0500, Tom Lane wrote:
>> I'm not convinced that this improves anything. The problem might not in
>> fact be either of the things you mention, in which case the new message
>> is outright misleading. Also, what of the policy stated in the header
>> comment for the function you're hacking, ie we intentionally don't reveal
>> the precise cause of the failure to the client?
> Well, the only solution then would be to add some weasel words like
> "perhaps expired password", but that seems so rare that I doubt it would
> apply very often and seems like an odd suggestion. We could go with:
> password authentication failed for user \"%s\": perhaps invalid or expired password
> We did have two threads on this issue in the past 12 months so I figured
> we should try to do something.
I agree with doing *something*, but this particular thing seems to violate
our very long-standing policy on how to deal with authentication failures,
as well as being too vague to be really useful.
What would be well within that policy is to log additional information
into the postmaster log. I see that md5_crypt_verify knows perfectly
well whether the problem is no password set, wrong password, or expired
password. I don't see anything wrong with having it emit a log entry
--- maybe not in the second case for fear of log-spam complaints, but
certainly the third case and maybe the first one. Or possibly cleaner,
have it return additional status so that auth_failed() can include the
info in the main ereport using errdetail_log().
regards, tom lane
I wrote:
> I agree with doing *something*, but this particular thing seems to violate
> our very long-standing policy on how to deal with authentication failures,
> as well as being too vague to be really useful.
> What would be well within that policy is to log additional information
> into the postmaster log. I see that md5_crypt_verify knows perfectly
> well whether the problem is no password set, wrong password, or expired
> password. I don't see anything wrong with having it emit a log entry
> --- maybe not in the second case for fear of log-spam complaints, but
> certainly the third case and maybe the first one. Or possibly cleaner,
> have it return additional status so that auth_failed() can include the
> info in the main ereport using errdetail_log().
Attached is a proposed patch that does exactly that. This just addresses
the cases mentioned above; once the infrastructure is in place, there
might be more things that would be worth logging using this mechanism.
regards, tom lane
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 882dc8f..1974b57 100644
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***************
*** 38,46 ****
*----------------------------------------------------------------
*/
static void sendAuthRequest(Port *port, AuthRequest areq);
! static void auth_failed(Port *port, int status);
static char *recv_password_packet(Port *port);
! static int recv_and_check_password_packet(Port *port);
/*----------------------------------------------------------------
--- 38,46 ----
*----------------------------------------------------------------
*/
static void sendAuthRequest(Port *port, AuthRequest areq);
! static void auth_failed(Port *port, int status, char *logdetail);
static char *recv_password_packet(Port *port);
! static int recv_and_check_password_packet(Port *port, char **logdetail);
/*----------------------------------------------------------------
*************** ClientAuthentication_hook_type ClientAut
*** 207,216 ****
* in use, and these are items that must be presumed known to an attacker
* anyway.
* Note that many sorts of failure report additional information in the
! * postmaster log, which we hope is only readable by good guys.
*/
static void
! auth_failed(Port *port, int status)
{
const char *errstr;
int errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION;
--- 207,217 ----
* in use, and these are items that must be presumed known to an attacker
* anyway.
* Note that many sorts of failure report additional information in the
! * postmaster log, which we hope is only readable by good guys. In
! * particular, if logdetail isn't NULL, we send that string to the log.
*/
static void
! auth_failed(Port *port, int status, char *logdetail)
{
const char *errstr;
int errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION;
*************** auth_failed(Port *port, int status)
*** 273,286 ****
}
if (port->hba)
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port->user_name),
! errdetail_log("Connection matched pg_hba.conf line %d: \"%s\"", port->hba->linenumber,
port->hba->rawline)));
! else
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port->user_name)));
/* doesn't return */
}
--- 274,294 ----
}
if (port->hba)
! {
! char *cdetail;
!
! cdetail = psprintf(_("Connection matched pg_hba.conf line %d: \"%s\""),
! port->hba->linenumber, port->hba->rawline);
! if (logdetail)
! logdetail = psprintf("%s\n%s", logdetail, cdetail);
! else
! logdetail = cdetail;
! }
!
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port->user_name),
! logdetail ? errdetail_log("%s", logdetail) : 0));
/* doesn't return */
}
*************** void
*** 294,299 ****
--- 302,308 ----
ClientAuthentication(Port *port)
{
int status = STATUS_ERROR;
+ char *logdetail = NULL;
/*
* Get the authentication method to use for this frontend/database
*************** ClientAuthentication(Port *port)
*** 507,518 ****
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled")));
sendAuthRequest(port, AUTH_REQ_MD5);
! status = recv_and_check_password_packet(port);
break;
case uaPassword:
sendAuthRequest(port, AUTH_REQ_PASSWORD);
! status = recv_and_check_password_packet(port);
break;
case uaPAM:
--- 516,527 ----
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg("MD5 authentication is not supported when \"db_user_namespace\" is enabled")));
sendAuthRequest(port, AUTH_REQ_MD5);
! status = recv_and_check_password_packet(port, &logdetail);
break;
case uaPassword:
sendAuthRequest(port, AUTH_REQ_PASSWORD);
! status = recv_and_check_password_packet(port, &logdetail);
break;
case uaPAM:
*************** ClientAuthentication(Port *port)
*** 552,558 ****
if (status == STATUS_OK)
sendAuthRequest(port, AUTH_REQ_OK);
else
! auth_failed(port, status);
/* Done with authentication, so we should turn off immediate interrupts */
ImmediateInterruptOK = false;
--- 561,567 ----
if (status == STATUS_OK)
sendAuthRequest(port, AUTH_REQ_OK);
else
! auth_failed(port, status, logdetail);
/* Done with authentication, so we should turn off immediate interrupts */
ImmediateInterruptOK = false;
*************** recv_password_packet(Port *port)
*** 680,688 ****
/*
* Called when we have sent an authorization request for a password.
* Get the response and check it.
*/
static int
! recv_and_check_password_packet(Port *port)
{
char *passwd;
int result;
--- 689,698 ----
/*
* Called when we have sent an authorization request for a password.
* Get the response and check it.
+ * On error, optionally store a detail string at *logdetail.
*/
static int
! recv_and_check_password_packet(Port *port, char **logdetail)
{
char *passwd;
int result;
*************** recv_and_check_password_packet(Port *por
*** 692,698 ****
if (passwd == NULL)
return STATUS_EOF; /* client wouldn't send password */
! result = md5_crypt_verify(port, port->user_name, passwd);
pfree(passwd);
--- 702,708 ----
if (passwd == NULL)
return STATUS_EOF; /* client wouldn't send password */
! result = md5_crypt_verify(port, port->user_name, passwd, logdetail);
pfree(passwd);
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 56b3ea8..5451db6 100644
*** a/src/backend/libpq/crypt.c
--- b/src/backend/libpq/crypt.c
***************
*** 29,36 ****
#include "utils/timestamp.h"
int
! md5_crypt_verify(const Port *port, const char *role, char *client_pass)
{
int retval = STATUS_ERROR;
char *shadow_pass,
--- 29,42 ----
#include "utils/timestamp.h"
+ /*
+ * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
+ * In the error case, optionally store a palloc'd string at *logdetail
+ * that will be sent to the postmaster log (but not the client).
+ */
int
! md5_crypt_verify(const Port *port, const char *role, char *client_pass,
! char **logdetail)
{
int retval = STATUS_ERROR;
char *shadow_pass,
*************** md5_crypt_verify(const Port *port, const
*** 58,63 ****
--- 64,71 ----
if (isnull)
{
ReleaseSysCache(roleTup);
+ *logdetail = psprintf(_("User \"%s\" has no password assigned."),
+ role);
return STATUS_ERROR; /* user has no password */
}
shadow_pass = TextDatumGetCString(datum);
*************** md5_crypt_verify(const Port *port, const
*** 148,154 ****
--- 156,166 ----
if (isnull)
retval = STATUS_OK;
else if (vuntil < GetCurrentTimestamp())
+ {
+ *logdetail = psprintf(_("User \"%s\" has an expired password."),
+ role);
retval = STATUS_ERROR;
+ }
else
retval = STATUS_OK;
}
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index 818e57e..b91024f 100644
*** a/src/include/libpq/crypt.h
--- b/src/include/libpq/crypt.h
***************
*** 15,21 ****
#include "libpq/libpq-be.h"
! extern int md5_crypt_verify(const Port *port, const char *user,
! char *client_pass);
#endif
--- 15,21 ----
#include "libpq/libpq-be.h"
! extern int md5_crypt_verify(const Port *port, const char *role,
! char *client_pass, char **logdetail);
#endif
On Fri, Jan 24, 2014 at 10:10:00AM -0500, Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > On Thu, Jan 23, 2014 at 10:39:34PM -0500, Tom Lane wrote:
> >> I'm not convinced that this improves anything. The problem might not in
> >> fact be either of the things you mention, in which case the new message
> >> is outright misleading. Also, what of the policy stated in the header
> >> comment for the function you're hacking, ie we intentionally don't reveal
> >> the precise cause of the failure to the client?
>
> > Well, the only solution then would be to add some weasel words like
> > "perhaps expired password", but that seems so rare that I doubt it would
> > apply very often and seems like an odd suggestion. We could go with:
>
> > password authentication failed for user \"%s\": perhaps invalid or expired password
>
> > We did have two threads on this issue in the past 12 months so I figured
> > we should try to do something.
>
> I agree with doing *something*, but this particular thing seems to violate
> our very long-standing policy on how to deal with authentication failures,
> as well as being too vague to be really useful.
>
> What would be well within that policy is to log additional information
> into the postmaster log. I see that md5_crypt_verify knows perfectly
> well whether the problem is no password set, wrong password, or expired
> password. I don't see anything wrong with having it emit a log entry
> --- maybe not in the second case for fear of log-spam complaints, but
> certainly the third case and maybe the first one. Or possibly cleaner,
> have it return additional status so that auth_failed() can include the
> info in the main ereport using errdetail_log().
I was afraid that PGOPTIONS='-c client_min_messages=log' would allow
clients to see the log messages, but in testing I found we don't show
them during authentication, and I found this C comment:
* client_min_messages is honored only after we complete the * authentication handshake. This is
requiredboth for security * reasons and because many clients can't handle NOTICE messages * during
authentication.
I like the 'LOG' idea very much, and liked your patch too. :-)
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ Everyone has their own god. +