Re: [Ejbca-develop] Samba smart card logon with EjbCA first step

[Branko M. <br...@ma...>]

On Mon, 06 May 2013 16:36:44 +0200
Gémes Géza <ge...@kz...> wrote:

> Hi,
> 
> The attached python script generates a certificate request and an
> info file to be submitted to EJBCA in order to generate an AD DC
> certificate for samba.
> It requires m2crypto and a samba (>=4.x) installation (no need for a 
> provision, it can be run remotely by specifying the name of the DC on 
> command line, then it asks for a username and a password).
> 
> Please write your comments/suggestions/patches about it.
> 
> I'm going forward to trying to code the certificate installation to
> the Samba DC.
> 
> Cheers
> 
> Geza Gemes

Hello Géza

Going through the script, seems to be ok, although I have no clue about
correctness of the Samba-related code, but you know what you're doing
there I believe :)

Take the below comments with a grain of salt, since I'm a bit nitpicky,
and this in no way means your code is bad. Not to mention it's always
easy to criticise other people's code :)

Of course, thanks for the contribution, it's really great to get more
instructions, scripts etc that help with integration of X.509 and
EJBCA!

Here's a couple of general remarks (most of them are PEP8 styling ones,
actually):

- [PEP8] Break-up import os, sys, socket, argparse into separate import
  lines.

- [PEP8] Use 4-space indentation instead of tabs.

- Don't hard-code Python path detection. It's better to let the user
  handle this (you could show informative message to user).

- [PEP8] Don't use lower-case class name (the getdc). Use CamelCase.
  I'd possibly name it a bit differently - I guess that one started off
  as a plain function?

- Move the 'import getpass' to the top (not much reason to have it in
  the middle of code).

- The LDAP is hard-coded to non-encrypted (no STARTTLS/SSL)
  connection. No idea how the SamDB handles it afterwards, though.

- If possible, you should add some explanation on the self.hexGUID=GUID
  line. Six months later when you get back to it you'll be thankful for
  it :)

- Existing files will be overwritten (.txt, .pem etc). No idea if it's
  worth it to make the code more resilient to this or not, though.

- Seeing that you're not defining any meaningful callback function
  for gen_key, you can completely drop it (by default it's None).

- RSA key length is hard-coded to 1024 bits. This might be a bit too
  low as well (2048 would be a saner value). Having an option to
  specify this would be a very good idea.

- Signing algorithm hash is hard-coded to SHA1 (then again, this might
  not be too big of a problem).

- There's not much error handling in the script. What happens if log-in
  to LDAP/domain fails?

- Small warning on argparse - that module is available starting
  with Python 2.7. On RHEL 6 they still have 2.6 series
  (unfortunately).

Best regards

-- 
Branko Majic
Jabber: br...@ma...
Please use only Free formats when sending attachments to me.

Бранко Мајић
Џабер: br...@ma...
Молим вас да додатке шаљете искључиво у слободним форматима.