11:52 p.m.
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
11:27 a.m.
On 2/21/2014 11:52 PM, Ade Lee wrote:
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
ACK for patch #198. Just some minor comments:
1. The get_class() may not be needed anymore.
2. Instead of json_value['KeyData'] should we use json_value.KeyData?
3. SymKeyGenerationRequest.DECRYPT_USAGE is kind of long and limited o
generation request. Maybe we should have something like
SymKey.DECRYPT_USAGE?
Still reviewing the other patches.
--
Endi S. Dewata
2:53 p.m.
On 2/21/2014 11:52 PM, Ade Lee wrote:
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
Patch #199:
1. The setup_database() throws a ValueError if the database already exists.
raise exceptions.ValueError(
"Directory already exists and over_write is false")
I'm not sure it's an appropriate exception:
http://docs.python.org/2/library/exceptions.html#exceptions.ValueError.
We probably could just throw a generic Exception.
2. The error message probably should just say "Directory already
exists". The end user should not see a variable name in the error
message. If the program ends with this error the user would know that an
existing database causes a conflict, so either they will remove it or
use a different path or overwrite it with a flag.
3. Since we're storing password, in NamedTemporaryFile() we probably
should specify some params to use user's temp dir instead of global temp
dir, and delete the temp file after closing. See:
http://docs.python.org/2/library/tempfile.html.
4. Minor thing. The data param in symmetric_unwrap() doc is followed by
a colon, which is inconsistent with the other params in that doc.
:param data: Data to be unwrapped
Is there a standard way of documenting params?
5. The "!=" operator could be replaced with "is not".
if transport_cert_nick is not None:
6. I think ideally client shouldn't be required to encode/decode the
trans_wrapped_session_key, wrappedPrivateData, nonceData,
transport_cert, etc. The base64 encoding should only be part of
marshalling/unmarshalling, hidden from the client. Thoughts?
7. Can these 3 invocations be combined:
cryptoutil.NSSCryptoUtil.setup_database(
certdb_dir, certdb_password, over_write=True)
crypto = cryptoutil.NSSCryptoUtil(certdb_dir, certdb_password)
crypto.initialize_db()
into this?
crypto = cryptoutil.NSSCryptoUtil(
certdb_dir, certdb_password, over_write=True)
Just a question, is it possible to use 2 different NSS databases at the
same time?
--
Endi S. Dewata
2:58 p.m.
On 2/21/2014 11:52 PM, Ade Lee wrote:
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
ACK for patch #200. Just one comment, get_key_info() throws
KeyNotFoundException and get_active_key_info() throws
ResourceNotFoundException. I think they should be consistent. Also
consider removing the resource-specific *NotFoundExceptions.
--
Endi S. Dewata
3:27 p.m.
On 2/21/2014 11:52 PM, Ade Lee wrote:
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
ACK for patch #201. Some comments:
1. Are we going to change the attribute name in the LDAP database? If so
do we need to upgrade the existing database?
2. The "client" options in the CLIs probably should be renamed to
"client-key".
3. In drmtest.py the label should be changed to "Client Key ID: ".
print "Client ID: " + str(key_info.clientKeyID)
4. Is this supposed to be removed?
#client_key_id = "Vek #1" + time.strftime('%X %x %Z')
client_key_id = "abcxyz"
5. Existing issue, the toString() is redundant.
clientKeyId = "UUID: 123-45-6789 VEK "
+ Calendar.getInstance().getTime().toString();
On the second thought, what do you think about using "key label" instead
of "client key ID"? So Key ID would remain the unique identifier for the
key, and the Key Label would be an identifier but it's not unique. No
need to redo the whole patch, just search & replace the patch. I can
help with this.
--
Endi S. Dewata
4:20 p.m.
I'll piggy back on this ACK. Just a few comments interspersed.
----- Original Message -----
From: "Endi Sukma Dewata" <edewata(a)redhat.com>
To: alee(a)redhat.com, pki-devel(a)redhat.com
Sent: Monday, February 24, 2014 12:53:37 PM
Subject: Re: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in
KeyResource server and client code.
On 2/21/2014 11:52 PM, Ade Lee wrote:
> Endi, Jack and I met to discuss various improvements to the
> Key/KeyResource client/server parts. Some of these are addressed in the
> attached patches. Some will be handled in separate tickets.
>
> Separate Tickets to be filed:
> 1. Add nonce mechanism for approvals.
> 2. Add openssl subclass for CryptoUtil
> 3. Extend generate_session_key() to return key in same call
> 4. Allow CLI to call python? (to be filed as separate ticket)
>
> Done in attached patches:
> 5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
> and extend to allow addition of trans_wrapped_session_key.
> 6. Add getActiveKey() to python client.
> 7. client_id -> client_key_id
> 8. constants in python API for key status
> 9. Add sanity checks to python client code
> 10. Move functions out of KRAClient.py and into key.py
> 11. from_dict() -> from)json()
> 12. Add methods to create nss certdb and import transport cert
> 13. All inputs/outputs from CryptoUtil are unencoded.
> 14. Fix usages in main function of SymKeyGenerationRequest
> 15. Fix bugs when retrieving invalid keyId.
> 16. Fix bugs when generating key with only clientID provided.
>
> To be done in next patch:
> 17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
> provide a more restricted convenience function generate_session_key()
>
> To be considered further:
> 1. rename session_key -> encryption_key/ wrapping_key?
> 2. revamp archival to not require client to generate PkiArchiveOptions
> object.
> 3. should retrieve functions return unwrapped key?
>
> Please review attached patches.
>
> Ade
Patch #199:
1. The setup_database() throws a ValueError if the database already exists.
raise exceptions.ValueError(
"Directory already exists and over_write is false")
I'm not sure it's an appropriate exception:
http://docs.python.org/2/library/exceptions.html#exceptions.ValueError.
We probably could just throw a generic Exception.
2. The error message probably should just say "Directory already
exists". The end user should not see a variable name in the error
message. If the program ends with this error the user would know that an
existing database causes a conflict, so either they will remove it or
use a different path or overwrite it with a flag.
For this db exists already? Is this really the end of the world?
Could we just go ahead and open it for them and move on? For instance
nss now has the notion of a completely common db where one could
conceivably just add to it.
3. Since we're storing password, in NamedTemporaryFile() we probably
should specify some params to use user's temp dir instead of global temp
dir, and delete the temp file after closing. See:
http://docs.python.org/2/library/tempfile.html.
4. Minor thing. The data param in symmetric_unwrap() doc is followed by
a colon, which is inconsistent with the other params in that doc.
:param data: Data to be unwrapped
Is there a standard way of documenting params?
5. The "!=" operator could be replaced with "is not".
if transport_cert_nick is not None:
6. I think ideally client shouldn't be required to encode/decode the
trans_wrapped_session_key, wrappedPrivateData, nonceData,
transport_cert, etc. The base64 encoding should only be part of
marshalling/unmarshalling, hidden from the client. Thoughts?
7. Can these 3 invocations be combined:
cryptoutil.NSSCryptoUtil.setup_database(
certdb_dir, certdb_password, over_write=True)
crypto = cryptoutil.NSSCryptoUtil(certdb_dir, certdb_password)
crypto.initialize_db()
into this?
I agree with this, if it's feasible. This way they don't have to worry about
initializing anything, they just put in the params to identify the db.
crypto = cryptoutil.NSSCryptoUtil(
certdb_dir, certdb_password, over_write=True)
Just a question, is it possible to use 2 different NSS databases at the
same time?
--
Endi S. Dewata
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
4:33 p.m.
ACK #200, comment below:
----- Original Message -----
From: "Endi Sukma Dewata" <edewata(a)redhat.com>
To: alee(a)redhat.com, pki-devel(a)redhat.com
Sent: Monday, February 24, 2014 12:58:08 PM
Subject: Re: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in
KeyResource server and client code.
On 2/21/2014 11:52 PM, Ade Lee wrote:
> Endi, Jack and I met to discuss various improvements to the
> Key/KeyResource client/server parts. Some of these are addressed in the
> attached patches. Some will be handled in separate tickets.
>
> Separate Tickets to be filed:
> 1. Add nonce mechanism for approvals.
> 2. Add openssl subclass for CryptoUtil
> 3. Extend generate_session_key() to return key in same call
> 4. Allow CLI to call python? (to be filed as separate ticket)
>
> Done in attached patches:
> 5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
> and extend to allow addition of trans_wrapped_session_key.
> 6. Add getActiveKey() to python client.
> 7. client_id -> client_key_id
> 8. constants in python API for key status
> 9. Add sanity checks to python client code
> 10. Move functions out of KRAClient.py and into key.py
> 11. from_dict() -> from)json()
> 12. Add methods to create nss certdb and import transport cert
> 13. All inputs/outputs from CryptoUtil are unencoded.
> 14. Fix usages in main function of SymKeyGenerationRequest
> 15. Fix bugs when retrieving invalid keyId.
> 16. Fix bugs when generating key with only clientID provided.
>
> To be done in next patch:
> 17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
> provide a more restricted convenience function generate_session_key()
>
> To be considered further:
> 1. rename session_key -> encryption_key/ wrapping_key?
> 2. revamp archival to not require client to generate PkiArchiveOptions
> object.
> 3. should retrieve functions return unwrapped key?
>
> Please review attached patches.
>
> Ade
ACK for patch #200. Just one comment, get_key_info() throws
KeyNotFoundException and get_active_key_info() throws
ResourceNotFoundException. I think they should be consistent. Also
consider removing the resource-specific *NotFoundExceptions.
But do the resource specific exceptions reveal more info as to what has not been found.
Will the generic one be able to convey that info? Just a question.
--
Endi S. Dewata
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
4:47 p.m.
ACK 201, comments below:
----- Original Message -----
From: "Endi Sukma Dewata" <edewata(a)redhat.com>
To: alee(a)redhat.com, pki-devel(a)redhat.com
Sent: Monday, February 24, 2014 1:27:21 PM
Subject: Re: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in KeyResource
server and client code.
On 2/21/2014 11:52 PM, Ade Lee wrote:
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
ACK for patch #201. Some comments:
1. Are we going to change the attribute name in the LDAP database? If so
do we need to upgrade the existing database?
I can see a case for leaving this the same. The user won't have big use
to look at the ldap.
2. The "client" options in the CLIs probably should be renamed to
"client-key".
3. In drmtest.py the label should be changed to "Client Key ID: ".
print "Client ID: " + str(key_info.clientKeyID)
4. Is this supposed to be removed?
#client_key_id = "Vek #1" + time.strftime('%X %x %Z')
client_key_id = "abcxyz"
5. Existing issue, the toString() is redundant.
clientKeyId = "UUID: 123-45-6789 VEK "
+ Calendar.getInstance().getTime().toString();
On the second thought, what do you think about using "key label" instead
of "client key ID"? So Key ID would remain the unique identifier for the
key, and the Key Label would be an identifier but it's not unique. No
need to redo the whole patch, just search & replace the patch. I can
help with this.
I think alee has done enough. The new name sounds fine to me. :) I don't think
having ID in the name automatically implies that the uniqueness on the key itself.
It's more for the client to identify a set of keys with one being the active one.
--
Endi S. Dewata
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
5:35 p.m.
----- Original Message -----
From: "Ade Lee" <alee(a)redhat.com>
To: pki-devel(a)redhat.com
Sent: Friday, February 21, 2014 9:52:18 PM
Subject: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in KeyResource
server and client code.
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
Patch #201
Looks like simple moving functionality from one class to another....
ACK
Just one caveat.
We now appear to have a "keys" property of the client to make calls having to do
with keys such as:
kraclient.keys.list_keys
I wonder if some of the methods of "KeyClient" now have a redundant portion of
"keys" in the method names.
Now that we know we are dealing with keys, perhaps some adjustment could be made there.
For instance
list_keys could become list.
5:36 p.m.
Sorry,
This is for patch #202.
----- Original Message -----
From: "John Magne" <jmagne(a)redhat.com>
To: alee(a)redhat.com
Cc: pki-devel(a)redhat.com
Sent: Monday, February 24, 2014 3:35:10 PM
Subject: Re: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in KeyResource
server and client code.
----- Original Message -----
From: "Ade Lee" <alee(a)redhat.com>
To: pki-devel(a)redhat.com
Sent: Friday, February 21, 2014 9:52:18 PM
Subject: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in KeyResource
server and client code.
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
Patch #201
Looks like simple moving functionality from one class to another....
ACK
Just one caveat.
We now appear to have a "keys" property of the client to make calls having to do
with keys such as:
kraclient.keys.list_keys
I wonder if some of the methods of "KeyClient" now have a redundant portion of
"keys" in the method names.
Now that we know we are dealing with keys, perhaps some adjustment could be made there.
For instance
list_keys could become list.
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
2:08 p.m.
On 2/24/2014 4:20 PM, John Magne wrote:
I'll piggy back on this ACK. Just a few comments interspersed.
> Patch #199:
>
> 2. The error message probably should just say "Directory already
> exists". The end user should not see a variable name in the error
> message. If the program ends with this error the user would know that an
> existing database causes a conflict, so either they will remove it or
> use a different path or overwrite it with a flag.
For this db exists already? Is this really the end of the world?
Could we just go ahead and open it for them and move on? For instance
nss now has the notion of a completely common db where one could
conceivably just add to it.
Not sure how up-to-date this page is, but here's what it says:
https://wiki.mozilla.org/NSS_Library_Init
"We intended to go to a single user/machine configuration for most
applications and libraries, ... One problem with this approach is that
not all NSS applications run on systems which will have a 'system
configured' NSS. In addition, there are still cases where the user may
want to keep multiple different configurations for testing ..."
I think there would be cases where the client wants to use a separate
database and make sure it's not overwriting an existing one.
> 7. Can these 3 invocations be combined:
>
> cryptoutil.NSSCryptoUtil.setup_database(
> certdb_dir, certdb_password, over_write=True)
> crypto = cryptoutil.NSSCryptoUtil(certdb_dir, certdb_password)
> crypto.initialize_db()
>
> into this?
I agree with this, if it's feasible. This way they don't have to worry about
initializing anything, they just put in the params to identify the db.
As discussed with Ade, it might not be possible now since importing the
transport cert is done using certutil instead of python-nss. Since the
nss database can't be shared among multiple processes, the import has to
be done first using certutil, then python-nss can intialize the database
after that.
Ideally the import has to be done with python-nss as well, but we don't
know if the functionality is exposed in python-nss.
--
Endi S. Dewata
2:11 p.m.
On 2/24/2014 4:33 PM, John Magne wrote:
ACK #200, comment below:
> ACK for patch #200. Just one comment, get_key_info() throws
> KeyNotFoundException and get_active_key_info() throws
> ResourceNotFoundException. I think they should be consistent. Also
> consider removing the resource-specific *NotFoundExceptions.
But do the resource specific exceptions reveal more info as to what has not been found.
Will the generic one be able to convey that info? Just a question.
So far the resource-specific exceptions only contain the resource ID and
the client already knows that since it's the one initiating the
operation. A generic ResourceNotFoundException will still contain the
resource ID as part of the error message.
--
Endi S. Dewata
2:16 p.m.
On 2/24/2014 4:47 PM, John Magne wrote:
ACK 201, comments below:
ACK for patch #201. Some comments:
1. Are we going to change the attribute name in the LDAP database? If so
do we need to upgrade the existing database?
I can see a case for leaving this the same. The user won't have big use
to look at the ldap.
Yeah, we don't have to upgrade the database now. But in the long term it
would be nice to have an attribute name that matches the client API.
On the second thought, what do you think about using "key
label" instead
of "client key ID"? So Key ID would remain the unique identifier for the
key, and the Key Label would be an identifier but it's not unique. No
need to redo the whole patch, just search & replace the patch. I can
help with this.
I think alee has done enough. The new name sounds fine to me. :) I
don't think
having ID in the name automatically implies that the uniqueness on the key itself.
It's more for the client to identify a set of keys with one being the active one.
Either name is fine. I just thought "label" is simpler. If we decided to
use label I can help make the changes, no extra work for alee :)
--
Endi S. Dewata
3:05 p.m.
On Mon, 2014-02-24 at 18:35 -0500, John Magne wrote:
----- Original Message -----
> From: "Ade Lee" <alee(a)redhat.com>
> To: pki-devel(a)redhat.com
> Sent: Friday, February 21, 2014 9:52:18 PM
> Subject: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in
KeyResource server and client code.
>
> Endi, Jack and I met to discuss various improvements to the
> Key/KeyResource client/server parts. Some of these are addressed in the
> attached patches. Some will be handled in separate tickets.
>
> Separate Tickets to be filed:
> 1. Add nonce mechanism for approvals.
> 2. Add openssl subclass for CryptoUtil
> 3. Extend generate_session_key() to return key in same call
> 4. Allow CLI to call python? (to be filed as separate ticket)
>
> Done in attached patches:
> 5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
> and extend to allow addition of trans_wrapped_session_key.
> 6. Add getActiveKey() to python client.
> 7. client_id -> client_key_id
> 8. constants in python API for key status
> 9. Add sanity checks to python client code
> 10. Move functions out of KRAClient.py and into key.py
> 11. from_dict() -> from)json()
> 12. Add methods to create nss certdb and import transport cert
> 13. All inputs/outputs from CryptoUtil are unencoded.
> 14. Fix usages in main function of SymKeyGenerationRequest
> 15. Fix bugs when retrieving invalid keyId.
> 16. Fix bugs when generating key with only clientID provided.
>
> To be done in next patch:
> 17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
> provide a more restricted convenience function generate_session_key()
>
> To be considered further:
> 1. rename session_key -> encryption_key/ wrapping_key?
> 2. revamp archival to not require client to generate PkiArchiveOptions
> object.
> 3. should retrieve functions return unwrapped key?
>
> Please review attached patches.
>
> Ade
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
Patch #201
Looks like simple moving functionality from one class to another....
ACK
Just one caveat.
We now appear to have a "keys" property of the client to make calls having to
do with keys such as:
kraclient.keys.list_keys
I wonder if some of the methods of "KeyClient" now have a redundant portion of
"keys" in the method names.
Now that we know we are dealing with keys, perhaps some adjustment could be made there.
For instance
list_keys could become list.
Not really, KeyClient contain methods to list and perform operations on
Keys and KeyRequests. You need the extra bits to see what you are
listing.
Note that in the latest code, I do something like this:
keyclient = kraclient.keys
keyclient.list_keys()
etc.
4:48 p.m.
On 2/21/2014 11:52 PM, Ade Lee wrote:
Endi, Jack and I met to discuss various improvements to the
Key/KeyResource client/server parts. Some of these are addressed in the
attached patches. Some will be handled in separate tickets.
Separate Tickets to be filed:
1. Add nonce mechanism for approvals.
2. Add openssl subclass for CryptoUtil
3. Extend generate_session_key() to return key in same call
4. Allow CLI to call python? (to be filed as separate ticket)
Done in attached patches:
5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
and extend to allow addition of trans_wrapped_session_key.
6. Add getActiveKey() to python client.
7. client_id -> client_key_id
8. constants in python API for key status
9. Add sanity checks to python client code
10. Move functions out of KRAClient.py and into key.py
11. from_dict() -> from)json()
12. Add methods to create nss certdb and import transport cert
13. All inputs/outputs from CryptoUtil are unencoded.
14. Fix usages in main function of SymKeyGenerationRequest
15. Fix bugs when retrieving invalid keyId.
16. Fix bugs when generating key with only clientID provided.
To be done in next patch:
17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
provide a more restricted convenience function generate_session_key()
To be considered further:
1. rename session_key -> encryption_key/ wrapping_key?
2. revamp archival to not require client to generate PkiArchiveOptions
object.
3. should retrieve functions return unwrapped key?
Please review attached patches.
Ade
Some comments for #203:
1. It looks like TypeError is more common to indicate missing argument
rather than ValueError:
http://stackoverflow.com/questions/8297526/proper-exception-to-raise-if-n...
2. It might be better for end users to see a descriptive error message
rather than variable name, for example:
raise ValueError("Request ID must be specified")
3. I don't think we should mask invalid data. If there's an error the
exception will provide information to troubleshoot the problem.
public Integer getKeySize() {
try {
return new Integer(attributes.get(KEY_SIZE));
} catch (NumberFormatException e) {
return null;
}
}
--
Endi S. Dewata
9:08 a.m.
Issues addressed and pushed to master.
On Tue, 2014-02-25 at 14:08 -0600, Endi Sukma Dewata wrote:
On 2/24/2014 4:20 PM, John Magne wrote:
> I'll piggy back on this ACK. Just a few comments interspersed.
>
>> Patch #199:
>>
>> 2. The error message probably should just say "Directory already
>> exists". The end user should not see a variable name in the error
>> message. If the program ends with this error the user would know that an
>> existing database causes a conflict, so either they will remove it or
>> use a different path or overwrite it with a flag.
>
> For this db exists already? Is this really the end of the world?
> Could we just go ahead and open it for them and move on? For instance
> nss now has the notion of a completely common db where one could
> conceivably just add to it.
Not sure how up-to-date this page is, but here's what it says:
https://wiki.mozilla.org/NSS_Library_Init
"We intended to go to a single user/machine configuration for most
applications and libraries, ... One problem with this approach is that
not all NSS applications run on systems which will have a 'system
configured' NSS. In addition, there are still cases where the user may
want to keep multiple different configurations for testing ..."
I think there would be cases where the client wants to use a separate
database and make sure it's not overwriting an existing one.
>> 7. Can these 3 invocations be combined:
>>
>> cryptoutil.NSSCryptoUtil.setup_database(
>> certdb_dir, certdb_password, over_write=True)
>> crypto = cryptoutil.NSSCryptoUtil(certdb_dir, certdb_password)
>> crypto.initialize_db()
>>
>> into this?
>
> I agree with this, if it's feasible. This way they don't have to worry
about
> initializing anything, they just put in the params to identify the db.
As discussed with Ade, it might not be possible now since importing the
transport cert is done using certutil instead of python-nss. Since the
nss database can't be shared among multiple processes, the import has to
be done first using certutil, then python-nss can intialize the database
after that.
Ideally the import has to be done with python-nss as well, but we don't
know if the functionality is exposed in python-nss.
9:09 a.m.
Pushed to master.
Removing resource specific exceptions will be relegated to another
patch.
On Tue, 2014-02-25 at 14:11 -0600, Endi Sukma Dewata wrote:
On 2/24/2014 4:33 PM, John Magne wrote:
>
> ACK #200, comment below:
>
>> ACK for patch #200. Just one comment, get_key_info() throws
>> KeyNotFoundException and get_active_key_info() throws
>> ResourceNotFoundException. I think they should be consistent. Also
>> consider removing the resource-specific *NotFoundExceptions.
>
> But do the resource specific exceptions reveal more info as to what has not been
found.
> Will the generic one be able to convey that info? Just a question.
So far the resource-specific exceptions only contain the resource ID and
the client already knows that since it's the one initiating the
operation. A generic ResourceNotFoundException will still contain the
resource ID as part of the error message.
9:10 a.m.
Sticking with client_key_id. Pushed to master.
On Tue, 2014-02-25 at 14:16 -0600, Endi Sukma Dewata wrote:
On 2/24/2014 4:47 PM, John Magne wrote:
> ACK 201, comments below:
>
> ACK for patch #201. Some comments:
>
> 1. Are we going to change the attribute name in the LDAP database? If so
> do we need to upgrade the existing database?
>
> I can see a case for leaving this the same. The user won't have big use
> to look at the ldap.
Yeah, we don't have to upgrade the database now. But in the long term it
would be nice to have an attribute name that matches the client API.
> On the second thought, what do you think about using "key label" instead
> of "client key ID"? So Key ID would remain the unique identifier for the
> key, and the Key Label would be an identifier but it's not unique. No
> need to redo the whole patch, just search & replace the patch. I can
> help with this.
> I think alee has done enough. The new name sounds fine to me. :) I don't think
> having ID in the name automatically implies that the uniqueness on the key itself.
> It's more for the client to identify a set of keys with one being the active
one.
Either name is fine. I just thought "label" is simpler. If we decided to
use label I can help make the changes, no extra work for alee :)
9:10 a.m.
Pushed to master.
On Tue, 2014-02-25 at 16:05 -0500, Ade Lee wrote:
On Mon, 2014-02-24 at 18:35 -0500, John Magne wrote:
>
>
>
> ----- Original Message -----
> > From: "Ade Lee" <alee(a)redhat.com>
> > To: pki-devel(a)redhat.com
> > Sent: Friday, February 21, 2014 9:52:18 PM
> > Subject: [Pki-devel] [PATCH] 198-203 patches to address multiple issues in
KeyResource server and client code.
> >
> > Endi, Jack and I met to discuss various improvements to the
> > Key/KeyResource client/server parts. Some of these are addressed in the
> > attached patches. Some will be handled in separate tickets.
> >
> > Separate Tickets to be filed:
> > 1. Add nonce mechanism for approvals.
> > 2. Add openssl subclass for CryptoUtil
> > 3. Extend generate_session_key() to return key in same call
> > 4. Allow CLI to call python? (to be filed as separate ticket)
> >
> > Done in attached patches:
> > 5. Change kraclient.generate_sym_key -> kraclient.generate_symmetric_key
> > and extend to allow addition of trans_wrapped_session_key.
> > 6. Add getActiveKey() to python client.
> > 7. client_id -> client_key_id
> > 8. constants in python API for key status
> > 9. Add sanity checks to python client code
> > 10. Move functions out of KRAClient.py and into key.py
> > 11. from_dict() -> from)json()
> > 12. Add methods to create nss certdb and import transport cert
> > 13. All inputs/outputs from CryptoUtil are unencoded.
> > 14. Fix usages in main function of SymKeyGenerationRequest
> > 15. Fix bugs when retrieving invalid keyId.
> > 16. Fix bugs when generating key with only clientID provided.
> >
> > To be done in next patch:
> > 17. Rewrite cryptoutil.generate_symmetric_key() to be more generic and
> > provide a more restricted convenience function generate_session_key()
> >
> > To be considered further:
> > 1. rename session_key -> encryption_key/ wrapping_key?
> > 2. revamp archival to not require client to generate PkiArchiveOptions
> > object.
> > 3. should retrieve functions return unwrapped key?
> >
> > Please review attached patches.
> >
> > Ade
> >
> >
> > _______________________________________________
> > Pki-devel mailing list
> > Pki-devel(a)redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
>
>
> Patch #201
>
> Looks like simple moving functionality from one class to another....
>
> ACK
>
> Just one caveat.
>
> We now appear to have a "keys" property of the client to make calls having
to do with keys such as:
>
> kraclient.keys.list_keys
>
> I wonder if some of the methods of "KeyClient" now have a redundant
portion of "keys" in the method names.
> Now that we know we are dealing with keys, perhaps some adjustment could be made
there. For instance
> list_keys could become list.
>
>
Not really, KeyClient contain methods to list and perform operations on
Keys and KeyRequests. You need the extra bits to see what you are
listing.
Note that in the latest code, I do something like this:
keyclient = kraclient.keys
keyclient.list_keys()
etc.
>
>
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
3724
days inactive
3728
days old
18 comments
3 participants
participants (3)
-
Ade Lee -
Endi Sukma Dewata -
John Magne