V3 #124
V3 #124
Conversation
SCAutolib/controller.py
Outdated
| packages += ["pcsc-lite-ccid", "pcsc-lite", "virt_cacard", | ||
| "vpcd", "softhsm"] | ||
| run("dnf -y copr enable jjelen/vsmartcard") | ||
|
|
||
| # Add IPA packages if needed | ||
| if not all([u["local"] for u in self.lib_conf["users"]]): | ||
| if not all([u["user_type"] == "local" for u in self.lib_conf["users"]]): |
SCAutolib/controller.py
Outdated
| logger.info(f"Local CA is configured in {ca_dir}") | ||
|
|
||
| dump_to_json(self.local_ca) | ||
|
|
||
| def setup_custom_ca(self, card_data: dict): | ||
| if card_data["card_type"] == "physical": |
There was a problem hiding this comment.
not sure what the flow for this method, but seems like card_data should be some object with card_type attribute, but not a dict
There was a problem hiding this comment.
Earlier in controller a .json file is deserialized and provided as dictionary to _validate_configuraton that returns another dictionary called 'self.lib_conf'. Part of this dictionary is is provided as a parameter here and I think approaching it as dict is correct here.
SCAutolib/controller.py
Outdated
| hsm_conf = file.SoftHSM2Conf(card_dir.joinpath("sofhtsm2.conf"), | ||
| card_dir=card_dir) | ||
| hsm_conf.create() | ||
| hsm_conf.save() | ||
|
|
||
| new_user.card = card.VirtualCard(new_user, | ||
| softhsm2_conf=hsm_conf.path) | ||
| new_card = card.VirtualCard(card_dict, softhsm2_conf=hsm_conf.path, | ||
| card_dir=card_dir) | ||
|
|
||
| # card needs to know some details of its user, so we add user as | ||
| # card attribute | ||
| for card_user in self.users: | ||
| if card_user.username == card_dict["cardholder"]: | ||
| new_card.user = card_user | ||
|
|
||
| if new_card.user.user_type == "local": | ||
| cnf_path = new_card.card_dir.joinpath(f"{new_card.cardholder}.cnf") | ||
| cnf = file.OpensslCnf(filepath=cnf_path, conf_type="user", | ||
| replace=[new_card.cardholder, new_card.CN]) | ||
| cnf.create() | ||
| cnf.save() | ||
|
|
||
| new_card.cnf = cnf.path | ||
|
|
||
| if force: | ||
| if new_card.cert and new_card.cert.exists(): | ||
| self.local_ca.revoke_cert(new_card.cert) | ||
| else: # IPA user | ||
| if force: | ||
| if new_card.cert and new_card.cert.exists(): | ||
| self.ipa_ca.revoke_cert(new_card.cert) |
There was a problem hiding this comment.
it would be nice to reduce the size of this single method and move this code to separate function just for better separation of concerns and potential testability
| CN: str = None | ||
| UID: str = None |
There was a problem hiding this comment.
python naming convention is to use lowercase for the class attributes
| @@ -161,10 +183,18 @@ def __exit__(self, exp_type, exp_value, exp_traceback): | |||
|
|
|||
| def to_dict(self): | |||
| return { | |||
There was a problem hiding this comment.
checkt what built-it method __dict__ would return, maybe it would smth you need
There was a problem hiding this comment.
Thanks for the suggestion, at this moment, I want to keep the content under control by assigning values manually.
Initial file structure Update paths to files in setup.py Update documentation for run function Fix return code of pytest if not tests are found
Add CA base interface ca.py Add implementation of the LocalCA class Add unit tests Add unit tests Update documentation Update metadata about package Add fixture for backping sssd ca db Update setup.py to include package data Update for review Update docs
Add unit tests for IPA server CA Disable auxiliary logs from not important loggers Update docs and logs Add test for force option Move IPAServerCA to CA.py Add assertion for making requests to IPA server via ClientMeta Update linter Fix renaming of the cert output Add unit test for IPA server CA Remove redundant test Update for review Remove hardcoded admin password Add propagation of default env values to pytest env
The module provides context managed function assert_log. It can assert, that a new log has been written in time between entering and exiting the context manager.
|
Hello @GeorgePantelakis , @Jakuje , @x00Pavel. Updated version of code reflecting most of previous comments is ready. Note, that unit tests are failing and I did not updated them to pass with V3 as some of them are destructive (changes system config files etc.) and should probably be reviewed and rewritten. |
mahavrila
force-pushed
the
V3
branch
2 times, most recently
from
d96f12c
to
35e0137
Compare
| new_card.cnf = self.prepare_user_cnf(new_card) | ||
| if force: | ||
| self.revoke_certs(new_card) | ||
| new_card.create() |
There was a problem hiding this comment.
What happens if force=false but the card (and in extension the card_dir) already exists? Is that a possibility?
There was a problem hiding this comment.
I assume that in such case it will use existing certificates - i.e. it will not create new certs later on. This could be useful if for instance one wants to use specific certs. These certs will not be erased if force=False.
| """ | ||
| self.name = LocalCA.ca_name |
There was a problem hiding this comment.
Why there are name and ca_name variables since they have the same value and it doesn't get changed? Perhaps only keep the name one?
There was a problem hiding this comment.
I wanted to declare ca_name that is the same for all instances of the class, however, later in code we use "dict.items" and If I remember correctly, this did not work on class varialbles so I have to add variable also to init. There we have self.name.
| @@ -343,6 +409,8 @@ def __init__(self, ip_addr: str, server_hostname: str, domain: str, | |||
| be used instead | |||
| :type realm: str | |||
| """ | |||
| self.ca_type = IPAServerCA.ca_type | |||
| self.name = IPAServerCA.ca_name | |||
There was a problem hiding this comment.
The same with ca_name and name as in LocalCA class.
There was a problem hiding this comment.
See previous reply.
Versioning is a mess, current version is called V2 internally, however, it is not reflected in git. Let me create V2 from current version.
New structure of config file is defined in schemas in the init file. Aim of new structure is to improve representation of smart cards and their details and separation od cards from users. As an example PIN and card_dir are no longer defined in user.
Allow to define and set CN of virtual smart card independenlty of username. Up until now username was used as CN. This is not practical mainly when comes physical cards where CN and username are predefined generally different calues
Each token used to be attribute of user resulting in loading card as a part of user object. As we need to separate user and ccards objects tokens will be loaded independently. Multiple token obj will be able to coexist.
Card object separates from user object. Many attributes are moved to card and User class is simplyfied to the extent that it can be unified with BaseUser class. Methods that were in User class are mostly moved to Card clases.
As classes for user, card and CA and also util.py were modified, controller needs to reflect these changes.
Add Classes and methods that represent physical cards and related changes in controller and utils
- asserts are replaced by explicit reises of exceptions - enums replace strings where possible - enums are organized in separate file - names of some methods and functons are updated - spelling and linting is improved - minor modification improving readability are introduced
Labels of virtual cards were populated with username of cardholder. This was misleading in logs and expect scripts. It will now be populated by name of card.
|
The code has been already merged. Closing PR. |
V3 of SCAutolib differs from V2 mostly in:
Users and cards are related only in prepare phase where objects are created. Card knows it's user. It's needed as some methods needs to know is user is local or kerberos user. Data of user and cards objects are stored separately.