diff mbox

[1/2] Support python 3 in utils/key2pub.py.

Message ID 1437525466-27512-2-git-send-email-ahmed.taahir@gmail.com (mailing list archive)
State Not Applicable
Delegated to: Kalle Valo
Headers show

Commit Message

ahmed.taahir@gmail.com July 22, 2015, 12:37 a.m. UTC
From: Taahir Ahmed <ahmed.taahir@gmail.com>

utils/key2pub.py can now be run under either python 2.7 or python 3.x.
This required some minor syntactical changes as well as switching from
M2Crypto to pycrypto, since M2Crypto doesn't support python 3.x.

In addition, some errors in the generated source file keys-ssl.h are
fixed:

  * The correct OpenSSL header for BN_ULONG is included.

  * The generated constants are given the 'ull' suffix to prevent
    warnings about constants that are too large.
---
 Makefile         |   2 +-
 utils/key2pub.py | 146 ++++++++++++++++++++++++++++---------------------------
 2 files changed, 75 insertions(+), 73 deletions(-)

Comments

Stefan Lippers-Hollmann July 22, 2015, 2:50 a.m. UTC | #1
Hi

On 2015-07-21, ahmed.taahir@gmail.com wrote:
> From: Taahir Ahmed <ahmed.taahir@gmail.com>
> 
> utils/key2pub.py can now be run under either python 2.7 or python 3.x.
> This required some minor syntactical changes as well as switching from
> M2Crypto to pycrypto, since M2Crypto doesn't support python 3.x.
[...]
> diff --git a/Makefile b/Makefile
> index a3ead30..65fc780 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -112,7 +112,7 @@ $(REG_BIN):
>  keys-%.c: utils/key2pub.py $(wildcard $(PUBKEY_DIR)/*.pem)
>  	$(NQ) '  GEN ' $@
>  	$(NQ) '  Trusted pubkeys:' $(wildcard $(PUBKEY_DIR)/*.pem)
> -	$(Q)./utils/key2pub.py --$* $(wildcard $(PUBKEY_DIR)/*.pem) $@
> +	$(Q) python /utils/key2pub.py --$* $(wildcard $(PUBKEY_DIR)/*.pem) $@
[...]

Without having tested it, nor being that familiar with python coding,
but this hunk seems to be problematic on two accords.

You omit $(pwd) from ./utils/key2pub.py, while /utils/key2pub.py
won't exist.

As little as I know about python packaging policies in Debian
(and probably Fedora), /usr/bin/python is never supposed to point
to python3 - afaik the interpreter should always be called python3
there, so I don't really see how that's going to work there.

Sorry if I missed anything obvious, but these things just caught
my attention without having looked any deeper.

Regards
	Stefan Lippers-Hollmann
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ahmed.taahir@gmail.com July 22, 2015, 2:59 a.m. UTC | #2
On Wednesday 22 July 2015 04:50:45 Stefan Lippers-Hollmann wrote:
> You omit $(pwd) from ./utils/key2pub.py, while /utils/key2pub.py
> won't exist.

Wow, that's a really bad mistake on my part.  It should indeed probably
be './utils/key2pub.py'.  I made this change after my general testing,
simply because emacs was undoing the execute bit every time I saved
key2pub.py.

It's not really a material change, so I'll probably just put it back the
way it was.

The rest of the patch is tested, I pinky-swear :)
 
> As little as I know about python packaging policies in Debian
> (and probably Fedora), /usr/bin/python is never supposed to point
> to python3 - afaik the interpreter should always be called python3
> there, so I don't really see how that's going to work there.

I'm not assuming that the system interpreter is any particular version:
key2pub.py has been modified so it runs under either 2.7 or 3.x.

Some more simplification might be possible if 2.7 support is dropped,
but not much, and I didn't want to rock the boat.

Thanks for the prompt review!

Taahir
Stefan Lippers-Hollmann July 22, 2015, 4:01 a.m. UTC | #3
Hi

On 2015-07-21, Taahir Ahmed wrote:
> On Wednesday 22 July 2015 04:50:45 Stefan Lippers-Hollmann wrote:
> > You omit $(pwd) from ./utils/key2pub.py, while /utils/key2pub.py
> > won't exist.
> 
> Wow, that's a really bad mistake on my part.  It should indeed probably
> be './utils/key2pub.py'.  I made this change after my general testing,
> simply because emacs was undoing the execute bit every time I saved
> key2pub.py.
> 
> It's not really a material change, so I'll probably just put it back the
> way it was.
> 
> The rest of the patch is tested, I pinky-swear :)
>  
> > As little as I know about python packaging policies in Debian
> > (and probably Fedora), /usr/bin/python is never supposed to point
> > to python3 - afaik the interpreter should always be called python3
> > there, so I don't really see how that's going to work there.
> 
> I'm not assuming that the system interpreter is any particular version:
> key2pub.py has been modified so it runs under either 2.7 or 3.x.
> 
> Some more simplification might be possible if 2.7 support is dropped,
> but not much, and I didn't want to rock the boat.

The problem, as I understand it, is that the python3 interpreter will
never be available as (/usr/bin/)python on Debian (or Fedora; even if
python2.x is not installed on the system), but always be called python3. 
So your new python3 compatibility is never actually used, neither on a 
python3-only system.

Therefore I'd suggest this approach instead, either make the python
interpreter user configurable, e.g.:

PYTHON ?= python

so the user can specify the interpreter when invoking make (as in
make PYTHON=python3), xor trying to auto-detect it (untested):

ifeq ($(shell which python3),)
  PYTHON = python
else
  PYTHON = python3
endif

and then using 

$(Q) $(PYTHON) ./utils/key2pub.py --$* $(wildcard $(PUBKEY_DIR)/*.pem) $@

wherever python is needed.

Regards
	Stefan Lippers-Hollmann
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Makefile b/Makefile
index a3ead30..65fc780 100644
--- a/Makefile
+++ b/Makefile
@@ -112,7 +112,7 @@  $(REG_BIN):
 keys-%.c: utils/key2pub.py $(wildcard $(PUBKEY_DIR)/*.pem)
 	$(NQ) '  GEN ' $@
 	$(NQ) '  Trusted pubkeys:' $(wildcard $(PUBKEY_DIR)/*.pem)
-	$(Q)./utils/key2pub.py --$* $(wildcard $(PUBKEY_DIR)/*.pem) $@
+	$(Q) python /utils/key2pub.py --$* $(wildcard $(PUBKEY_DIR)/*.pem) $@
 
 $(LIBREG): regdb.h reglib.h reglib.c
 	$(NQ) '  CC  ' $@
diff --git a/utils/key2pub.py b/utils/key2pub.py
index 3e84cd2..ff92748 100755
--- a/utils/key2pub.py
+++ b/utils/key2pub.py
@@ -1,126 +1,128 @@ 
 #!/usr/bin/env python
 
+import io
 import sys
 try:
-       from M2Crypto import RSA
-except ImportError, e:
-       sys.stderr.write('ERROR: Failed to import the "M2Crypto" module: %s\n' % e.message)
-       sys.stderr.write('Please install the "M2Crypto" Python module.\n')
-       sys.stderr.write('On Debian GNU/Linux the package is called "python-m2crypto".\n')
-       sys.exit(1)
+    from Crypto.PublicKey import RSA
+except ImportError as e:
+    sys.stderr.write('ERROR: Failed to import the "Crypto.PublicKey" module: %s\n' % e.message)
+    sys.stderr.write('Please install the "Crypto.PublicKey" Python module.\n')
+    sys.stderr.write('On Debian GNU/Linux the package is called "python-crypto".\n')
+    sys.exit(1)
+
+def bitwise_collect(value, radix_bits):
+    words = []
+    radix_mask = (1 << radix_bits) - 1
+    while value != 0:
+        words.append(value & radix_mask)
+        value >>= radix_bits
+    return words
 
 def print_ssl_64(output, name, val):
-    while val[0] == '\0':
-        val = val[1:]
-    while len(val) % 8:
-        val = '\0' + val
-    vnew = []
-    while len(val):
-        vnew.append((val[0], val[1], val[2], val[3], val[4], val[5], val[6], val[7]))
-        val = val[8:]
-    vnew.reverse()
-    output.write('static BN_ULONG %s[%d] = {\n' % (name, len(vnew)))
+    # OpenSSL expects 64-bit words given least-significant-word first.
+    vwords = bitwise_collect(val, 64)
+
+    output.write(u'static BN_ULONG {}[] = {{\n'.format(name))
     idx = 0
-    for v1, v2, v3, v4, v5, v6, v7, v8 in vnew:
+    for vword in vwords:
         if not idx:
-            output.write('\t')
-        output.write('0x%.2x%.2x%.2x%.2x%.2x%.2x%.2x%.2x, ' % (ord(v1), ord(v2), ord(v3), ord(v4), ord(v5), ord(v6), ord(v7), ord(v8)))
+            output.write(u'\t')
+        output.write(u'0x{:016x}ULL, '.format(vword))
         idx += 1
         if idx == 2:
             idx = 0
-            output.write('\n')
+            output.write(u'\n')
     if idx:
-        output.write('\n')
-    output.write('};\n\n')
+        output.write(u'\n')
+    output.write(u'};\n\n')
 
 def print_ssl_32(output, name, val):
-    while val[0] == '\0':
-        val = val[1:]
-    while len(val) % 4:
-        val = '\0' + val
-    vnew = []
-    while len(val):
-        vnew.append((val[0], val[1], val[2], val[3], ))
-        val = val[4:]
-    vnew.reverse()
-    output.write('static BN_ULONG %s[%d] = {\n' % (name, len(vnew)))
+    # OpenSSL expects 32-bit words given least-significant-word first.
+    vwords = bitwise_collect(val, 32)
+
+    output.write(u'static BN_ULONG {}[] = {{\n'.format(name))
     idx = 0
-    for v1, v2, v3, v4 in vnew:
+    for vword in vwords:
         if not idx:
-            output.write('\t')
-        output.write('0x%.2x%.2x%.2x%.2x, ' % (ord(v1), ord(v2), ord(v3), ord(v4)))
+            output.write(u'\t')
+        output.write(u'0x{:08x}, '.format(vword))
         idx += 1
         if idx == 4:
             idx = 0
-            output.write('\n')
+            output.write(u'\n')
     if idx:
-        output.write('\n')
-    output.write('};\n\n')
+        output.write(u'\n')
+    output.write(u'};\n\n')
 
 def print_ssl(output, name, val):
+
+    output.write(u'#include <stdint.h>\n')
+    output.write(u'#include <openssl/bn.h>\n')
+
     import struct
-    output.write('#include <stdint.h>\n')
     if len(struct.pack('@L', 0)) == 8:
         return print_ssl_64(output, name, val)
     else:
         return print_ssl_32(output, name, val)
 
 def print_ssl_keys(output, n):
-    output.write(r'''
+    output.write(u'''
 struct pubkey {
 	struct bignum_st e, n;
 };
 
-#define KEY(data) {				\
-	.d = data,				\
-	.top = sizeof(data)/sizeof(data[0]),	\
+#define KEY(data) {                          \\
+	.d = data,                           \\
+	.top = sizeof(data)/sizeof(data[0]), \\
 }
 
-#define KEYS(e,n)	{ KEY(e), KEY(n), }
+#define KEYS(e,n)    { KEY(e), KEY(n), }
 
 static struct pubkey keys[] = {
 ''')
     for n in xrange(n + 1):
-        output.write('	KEYS(e_%d, n_%d),\n' % (n, n))
-    output.write('};\n')
+        output.write(u'	KEYS(e_{0}, n_{0}),\n'.format(n))
+    output.write(u'};\n')
     pass
 
 def print_gcrypt(output, name, val):
-    output.write('#include <stdint.h>\n')
-    while val[0] == '\0':
-        val = val[1:]
-    output.write('static const uint8_t %s[%d] = {\n' % (name, len(val)))
+    # gcrypt expects 8-bit words most-significant-word first
+    vwords = bitwise_collect(val, 8)
+    vwords.reverse()
+    
+    output.write(u'#include <stdint.h>\n')
+    output.write(u'static const uint8_t %s[%d] = {\n' % (name, len(vwords)))
     idx = 0
-    for v in val:
+    for vword in vwords:
         if not idx:
-            output.write('\t')
-        output.write('0x%.2x, ' % ord(v))
+            output.write(u'\t')
+        output.write(u'0x{:02x}, '.format(vword))
         idx += 1
         if idx == 8:
             idx = 0
-            output.write('\n')
+            output.write(u'\n')
     if idx:
-        output.write('\n')
-    output.write('};\n\n')
+        output.write(u'\n')
+    output.write(u'};\n\n')
 
 def print_gcrypt_keys(output, n):
-    output.write(r'''
+    output.write(u'''
 struct key_params {
 	const uint8_t *e, *n;
 	uint32_t len_e, len_n;
 };
 
-#define KEYS(_e, _n) {			\
-	.e = _e, .len_e = sizeof(_e),	\
-	.n = _n, .len_n = sizeof(_n),	\
+#define KEYS(_e, _n) {                \\
+	.e = _e, .len_e = sizeof(_e), \\
+	.n = _n, .len_n = sizeof(_n), \\
 }
 
 static const struct key_params keys[] = {
 ''')
-    for n in xrange(n + 1):
-        output.write('	KEYS(e_%d, n_%d),\n' % (n, n))
-    output.write('};\n')
-    
+    for n in range(n + 1):
+        output.write(u'	KEYS(e_{0}, n_{0}),\n'.format(n))
+    output.write(u'};\n')
+
 
 modes = {
     '--ssl': (print_ssl, print_ssl_keys),
@@ -135,21 +137,21 @@  except IndexError:
     mode = None
 
 if not mode in modes:
-    print 'Usage: %s [%s] input-file... output-file' % (sys.argv[0], '|'.join(modes.keys()))
+    print('Usage: {} [{}] input-file... output-file'.format(sys.argv[0], '|'.join(modes.keys())))
     sys.exit(2)
 
-output = open(outfile, 'w')
+output = io.open(outfile, 'w')
 
 # load key
 idx = 0
 for f in files:
-    try:
-        key = RSA.load_pub_key(f)
-    except RSA.RSAError:
-        key = RSA.load_key(f)
 
-    modes[mode][0](output, 'e_%d' % idx, key.e[4:])
-    modes[mode][0](output, 'n_%d' % idx, key.n[4:])
+    key_contents = io.open(f, 'rb').read()
+    key = RSA.importKey(key_contents)
+
+    modes[mode][0](output, 'e_{}'.format(idx), key.e)
+    modes[mode][0](output, 'n_{}'.format(idx), key.n)
+
     idx += 1
 
 modes[mode][1](output, idx - 1)