Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.92) (envelope-from ) id 1jE9fB-0000fZ-Kt for pgadmin-hackers@arkaria.postgresql.org; Tue, 17 Mar 2020 10:41:33 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1jE9fA-0004V9-38 for pgadmin-hackers@arkaria.postgresql.org; Tue, 17 Mar 2020 10:41:32 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1jE9f9-0004V2-Rd for pgadmin-hackers@lists.postgresql.org; Tue, 17 Mar 2020 10:41:31 +0000 Received: from mail-vs1-xe41.google.com ([2607:f8b0:4864:20::e41]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jE9f7-0005RJ-3B for pgadmin-hackers@postgresql.org; Tue, 17 Mar 2020 10:41:31 +0000 Received: by mail-vs1-xe41.google.com with SMTP id y138so5690081vsy.0 for ; Tue, 17 Mar 2020 03:41:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=pGEbueBeUWdIqSBDW56hK09ifrwHFtGsStErLVcwWHs=; b=UELvayPsHT21QXsSFaWL8CUiSkzU+GC8M/yc1ad41LEojV6UCj0pXhuqlhSOzy9vzo PGqWANrS1uITSx9ECT5YBw5yC29lVyd99KL3/BUCUQZOdpNG6UxOW7ogC5xyjWcoIVLE sjWExSvqxv432zBmHMkOfC/+wfojtwIh2rjB2cKy+U4FeCkBVKZNAJ4FuTD9jivXcxsY H6AL5HVmBVD98vWe1v0EOohjYO8hYwzTuDq9yGqxBjFs0vccMfDeACS/7kKvhA2PMomz 2fS0YM7oFwCS4MTbDmoVlTGE098zl1SjUMjz4EskpXbWowsYHKVI20FWTmidUIkGNErc oY0g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=pGEbueBeUWdIqSBDW56hK09ifrwHFtGsStErLVcwWHs=; b=ON0Ub2AV9cloEQghM4WHVJN4JAxLUEHB9Kroig0Ieamzv2V+vDCmswZg3ynVwjfwj8 /jAJQSuSDFmuvKFd9REWc6ouCGi62eIwlT4mY39nNU2np0KbG+UZMKewcA3g2EbZlS8P tOU2six8/8m9BRJlWLq657svGJ9oLP8Q5IwP1wW0nylo/pKCCf0xrWtSeMsl3t3U3R9p 0LHrd9HErR4d667A0Xu8tkPkyo3eq1fsv0SOa6iljF9PySjdKg5FWlku1OMC4Cq24FrE gDHdqNgSurm29KZuHyDoDnlznG1QeU4AkQ8dDw+QGPd+LRVlhUZkrKCVaI9bLnKEcoZI R4Mg== X-Gm-Message-State: ANhLgQ3Uh07G22AgdnjQJvUI380WlOhsU5SCUbFshHPEbPf7mC2kxhpZ CreN1m9vuJs8ngy3+5OwWfSWflLU6uBBGRyC6vR+NQ== X-Google-Smtp-Source: ADFU+vsxnEtYzKPgYF9QLzlE5qwPPYOF4S2Rci5A8GsHZ7ty28bGthgunRXzzX500zOt+nB6V+OZ78Ap60ipjTXF8iQ= X-Received: by 2002:a67:33cb:: with SMTP id z194mr3015658vsz.155.1584441686867; Tue, 17 Mar 2020 03:41:26 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Tue, 17 Mar 2020 10:41:15 +0000 Message-ID: Subject: Re: [pgAdmin4][Patch] - RM 2186 - Support external authentication sources [LDAP] To: Khushboo Vashi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000e9d6df05a10a9338" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000e9d6df05a10a9338 Content-Type: text/plain; charset="UTF-8" Hi On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi < khushboo.vashi@enterprisedb.com> wrote: > Hi Dave, > > Thanks for the review. > > On Tue, Mar 17, 2020 at 3:42 PM Dave Page wrote: > >> Hi >> >> 30 second read of the first version of the patch... >> >> - Please move the configuration into config.py. Users should never have >> to modify a distributed file (it messes up packaging). I don't see any >> reason to use a different file just for auth config. >> >> There are many settings for the LDAP, and in the future we will add other > external sources also, so I thought it would be better if we have different > file for the authentication. > Sure, but our config file is small compared to many. Splitting things out is more confusing for users. If they want to do that themselves of course, they can add a config_local.py file which includes other files as needed. > - I think all config options should be prefixed with LDAP_ as we may have >> things like CERT_FILE for other purposes too. >> >> Sure. > >> - I don't see any test cases. >> >> I will think about this, as right now no idea how to write test cases for > this. > It should be fairly straightforward to write tests for some of the functions in the auth classes. For testing the actual LDAP stuff, we probably need to add LDAP config options to test_config.json, and only if present, run the tests. That would probably need to support a list of LDAP servers, so we can test with different configurations (LDAP, LDAPS, LDAP_STARTTLS, AD etc). > Thanks. >> >> Thanks, > Khushboo > >> >> On Tue, Mar 17, 2020 at 8:55 AM Khushboo Vashi < >> khushboo.vashi@enterprisedb.com> wrote: >> >>> Hi, >>> >>> Please find the attached patch to support LDAP Authentication in Server >>> mode. >>> To test the patch, config_auth.py needs to be configured for LDAP >>> configurations. The config settings are explained in this file in detail. >>> After configuring the parameters, start the pgadmin server in Server mode >>> and connect with LDAP server with the valid user via login page. >>> >>> I have tested this patch with ldap and ldap + ssl/tls. With the TLS, I >>> have used the default config of ldap3 without certificates. >>> >>> @Dave, can you please review this patch, as you have a better >>> understanding of LDAP and you can easily pointed out if I have missed >>> anything. >>> >>> Note: For the document update I will create the task and assign to Nidhi >>> for the same. >>> >>> Thanks, >>> Khushboo >>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --000000000000e9d6df05a10a9338 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <= ;khushboo.vashi@enterpri= sedb.com> wrote:
Hi Dave,

Thanks for = the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <dpage@pgadmin.org> wrote:
Hi
30 second read of the first version of the patch...
=

- Please move the configuration into config.py. Users s= hould never have to modify a distributed file (it messes up packaging). I d= on't see any reason to use a different file just for auth config.
=

There are many settings for the LDAP= , and in the future we will add other external sources also, so I thought i= t would be better if we have different file for the authentication.

Sure, but our config file is sma= ll compared to many. Splitting things out is more confusing for users. If t= hey want to do that themselves of course, they can add a config_local.py fi= le which includes other files as needed.
=C2=A0
<= div>
- I think all config options should be prefixed with LDAP_ a= s we may have things like CERT_FILE for other purposes too.

<= /div>
Sure.=C2=A0
- I don't see any= test cases.

I will think about= this, as right now no idea how to write test cases for this.=C2=A0

It should be fairly straightforw= ard to write tests for some of the functions in the auth classes. For testi= ng the actual LDAP stuff, we probably need to add LDAP config options to te= st_config.json, and only if present, run the tests. That would probably nee= d to support a list of LDAP servers, so we can test with different configur= ations (LDAP, LDAPS, LDAP_STARTTLS, AD etc).
=C2=A0
Thanks.

Than= ks,
Khushboo=C2=A0

On Tue, Mar 17, 2020 at 8:55 AM Khus= hboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi,

=
Please find the attached patch to support LDAP Authentication in Serve= r mode.
To test the patch, config_auth.py needs to be configured = for LDAP configurations. The config settings are explained in this file in = detail. After configuring the parameters, start the pgadmin server in Serve= r mode and connect with LDAP server with the valid user via login page.

I have tested this patch with ldap and ldap=C2=A0+ ss= l/tls. With the TLS, I have used the default config of ldap3 without certif= icates.

@Dave, can you please review this patch, a= s you have a better understanding of LDAP and you can easily pointed out if= I have missed anything.

Note: For the document up= date I will create the task and assign to Nidhi for the same.

Thanks,
Khushboo


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB= UK: http://www.e= nterprisedb.com
The Enterprise PostgreSQL Company


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @p= gsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL = Company
--000000000000e9d6df05a10a9338--