Add initial Domains REST API endpoint #17

f-trivino · 2023-01-12T17:20:18Z

This PR implements an API endpoint that allows making GET, POST, PUT, and PATCH requests to read, create, and update Domain objects.

This implementation includes helper functions in utils.py responsible for configuring SSSD service accordingly.

src/ipa-tuura/domains/utils.py

flo-renaud

Hi @f-trivino
please find some inline comments.
I didn't manage to run the delete domain part, or get domain. Not sure what is missing, though...

src/ipa-tuura/domains/models.py

src/install/requirements.txt

src/ipa-tuura/domains/views.py

src/ipa-tuura/domains/utils.py

src/ipa-tuura/ipatuura/ipa.py

This commit is also adding packages to requirements.txt for the Domains REST API Based on: https://www.django-rest-framework.org/#installation Signed-off-by: Francisco Trivino <[email protected]>

.github/workflows/gating.yaml

flo-renaud · 2023-02-17T15:04:48Z

Hi @f-trivino
When I add or remove a domain I see the following errors in the django console:

Internal Server Error: /domains/v1/domain/
Traceback (most recent call last):
  File "/usr/lib/python3.10/site-packages/ipalib/rpc.py", line 644, in get_auth_info
    response = self._sec_context.step()
  File "/usr/lib/python3.10/site-packages/decorator.py", line 232, in fun
    return caller(func, *(extras + args), **kw)
  File "/usr/lib64/python3.10/site-packages/gssapi/_utils.py", line 155, in check_last_err
    return func(self, *args, **kwargs)
  File "/usr/lib/python3.10/site-packages/decorator.py", line 232, in fun
    return caller(func, *(extras + args), **kw)
  File "/usr/lib64/python3.10/site-packages/gssapi/_utils.py", line 128, in catch_and_return_token
    return func(self, *args, **kwargs)
  File "/usr/lib64/python3.10/site-packages/gssapi/sec_contexts.py", line 523, in step
    return self._initiator_step(token=token)
  File "/usr/lib64/python3.10/site-packages/gssapi/sec_contexts.py", line 539, in _initiator_step
    res = rsec_contexts.init_sec_context(self._target_name, self._creds,
  File "gssapi/raw/sec_contexts.pyx", line 246, in gssapi.raw.sec_contexts.init_sec_context
gssapi.raw.exceptions.MissingCredentialsError: Major (458752): No credentials were supplied, or the credentials were unavailable or inaccessible, Minor (2529639053): No Kerberos credentials available (default cache: KCM:)

The code is probably missing a step after ipa-client-install in order to create the admin keytab.

flo-renaud

Hi @f-trivino
please find my first comments

src/ipa-tuura/domains/views.py

src/ipa-tuura/domains/utils.py

flo-renaud · 2023-02-17T16:51:43Z

src/ipa-tuura/domains/models.py

+    description = models.TextField(blank=True)
+
+    # The connection URL to the identity server
+    integration_domain_url = models.CharField(max_length=255)


If the provider is IPA, is this field optional? With the autodiscovery mechanism it is not required.

good catch, it is mandatory always. Would it be ok to fix this in next PRs?

flo-renaud · 2023-02-17T17:03:37Z

src/ipa-tuura/domains/utils.py

+
+    # container image should contain the user and group
+    # groupadd scim
+    args = ['groupadd', 'scim']


What is the purpose of this local user "scim"? Is it only to store the keytab for ipatuura->ipa communication?

yep, that's right, folder: /var/lib/ipa/ipatuura/

flo-renaud

Hi @f-trivino

I tested mainly the ipa provider and will continue tomorrow with ad and ldap providers. First comments:

right now, the Domains rest api does not require any authentication but it should be protected and restricted to the django admin user.
a call to delete a domain with a wrong id does not return any error, IMO it should mention something like "No such domain".
if a wrong client id/password is provided, ipa-client-install fails but this error is not well handled
if 2 calls are done to add the same domain, the rest api does not complain but we end up with 2 domains in the sql db and only one in sssd.conf. Removing any of these domains results in an inconsistency (one remaining domain in sql, but no domain in sssd.conf).

flo-renaud · 2023-03-15T16:53:02Z

src/ipa-tuura/domains/models.py

+    description = models.TextField(blank=True)
+
+    # The connection URL to the identity server
+    integration_domain_url = models.CharField(max_length=255)


flo-renaud

Hi @f-trivino
please find additional comments for LDAP and AD providers

flo-renaud · 2023-03-16T15:35:22Z

src/ipa-tuura/domains/utils.py

+    else:
+        if domain['id_provider'] == 'ad':
+            join_ad_realm(domain)
+        config_default_sssd(domain)


Calling realm join already creates sssd.conf, with different settings:

[sssd] config_file_version = 2 domains = ad.test services = nss, pam, ifp [nss] timeout = 60 [pam] timeout = 60 [ifp] user_attributes = +sn, +givenname, +lock, +mail [domain/ad.test] default_shell = /bin/bash krb5_store_password_if_offline = True cache_credentials = True krb5_realm = AD.TEST realmd_tags = manages-system joined-with-adcli id_provider = ad fallback_homedir = /home/%u@%d ad_domain = ad.test use_fully_qualified_names = True ldap_id_mapping = True access_provider = ad

Not sure that this call to config_default_sssd is needed.

For the ldap provider, which authentication method is used? it's possible to set ldap_krb5_keytab or ldap_default_bind_dn but I didn't find if ipatuura is configuring any.

The code right now is performing a simple bind with password. This needs to be improved, following issue ticket #28 talks about enabling TLS support.

I fixed sssd.conf and now ldap_default_bind_dn is added. I'm re-using client_id content for the bind_dn for now.

f-trivino · 2023-03-17T08:47:24Z

Hi @f-trivino

I tested mainly the ipa provider and will continue tomorrow with ad and ldap providers. First comments:

* right now, the Domains rest api does not require any authentication but it should be protected and restricted to the django admin user.

* a call to delete a domain with a wrong id does not return any error, IMO it should mention something like "No such domain".

* if a wrong client id/password is provided, ipa-client-install fails but this error is not well handled

* if 2 calls are done to add the same domain, the rest api does not complain but we end up with 2 domains in the sql db and only one in sssd.conf. Removing any of these domains results in an inconsistency (one remaining domain in sql, but no domain in sssd.conf).

Thanks @flo-renaud , I've filed tickets to address these issues:
#23
#24
#25

This commit implements an API endpoint that allows making GET, POST, PUT, and PATCH requests to read, create, and update Domain objects. This implementation includes helper functions in utils.py responsible for configuring sssd service accordingly. Signed-off-by: Francisco Trivino <[email protected]>

This commit adds the migrations based on the Domains model. Signed-off-by: Francisco Trivino <[email protected]>

This commit adds swagger as a documentation generator for REST framework as well as an easy way to test the domains administrative end-points. Signed-off-by: Francisco Trivino <[email protected]>

This commit adds missing code to extend writable interface to write back user modifications for attributes defined in AD integration domain. Signed-off-by: Francisco Trivino <[email protected]>

flo-renaud

Ok for merging, will open tickets for any discovered issue

f-trivino commented Jan 12, 2023

View reviewed changes

src/ipa-tuura/domains/utils.py Outdated Show resolved Hide resolved

f-trivino force-pushed the domains branch from 9770d1a to 178e6bd Compare January 13, 2023 12:04

f-trivino mentioned this pull request Jan 13, 2023

WIP: implement a new django app as a REST API to handle integration domains #3

Closed

6 tasks

f-trivino force-pushed the domains branch 3 times, most recently from ecd6d02 to a5d33f9 Compare January 13, 2023 12:25

f-trivino changed the title ~~WIP Add initial Domains REST API endpoint~~ Add initial Domains REST API endpoint Jan 13, 2023

f-trivino force-pushed the domains branch from a5d33f9 to 82f2ee1 Compare January 13, 2023 13:03

flo-renaud reviewed Jan 17, 2023

View reviewed changes

f-trivino force-pushed the domains branch 2 times, most recently from 86120d2 to dd73e58 Compare January 25, 2023 19:22

f-trivino force-pushed the domains branch from dd73e58 to 6fb91f7 Compare February 1, 2023 22:08

f-trivino mentioned this pull request Feb 1, 2023

Extend writable interface to support AD provider #18

Closed

f-trivino force-pushed the domains branch from 3ea921f to 33d4129 Compare February 2, 2023 11:56

f-trivino commented Feb 3, 2023

View reviewed changes

src/ipa-tuura/ipatuura/ipa.py Outdated Show resolved Hide resolved

f-trivino force-pushed the domains branch 9 times, most recently from 36db1d8 to dfcc725 Compare February 16, 2023 17:46

Use fedora38 container to run flake8 and unit tests

28d6020

This commit is also adding packages to requirements.txt for the Domains REST API Based on: https://www.django-rest-framework.org/#installation Signed-off-by: Francisco Trivino <[email protected]>

f-trivino force-pushed the domains branch from dfcc725 to e922721 Compare February 16, 2023 21:15

f-trivino commented Feb 16, 2023

View reviewed changes

.github/workflows/gating.yaml Show resolved Hide resolved

flo-renaud reviewed Feb 17, 2023

View reviewed changes

src/ipa-tuura/domains/views.py Outdated Show resolved Hide resolved

src/ipa-tuura/domains/views.py Outdated Show resolved Hide resolved

flo-renaud reviewed Feb 17, 2023

View reviewed changes

src/ipa-tuura/domains/utils.py Show resolved Hide resolved

flo-renaud reviewed Feb 17, 2023

View reviewed changes

f-trivino force-pushed the domains branch 4 times, most recently from bbbfa08 to 6651045 Compare March 15, 2023 07:51

flo-renaud reviewed Mar 15, 2023

View reviewed changes

flo-renaud reviewed Mar 16, 2023

View reviewed changes

f-trivino force-pushed the domains branch 2 times, most recently from 4d7b310 to f454a30 Compare March 22, 2023 21:53

f-trivino added 4 commits March 23, 2023 12:34

Add Domains app migrations

7fbd947

This commit adds the migrations based on the Domains model. Signed-off-by: Francisco Trivino <[email protected]>

Add swagger to generate Domains API documentation

e1aa357

This commit adds swagger as a documentation generator for REST framework as well as an easy way to test the domains administrative end-points. Signed-off-by: Francisco Trivino <[email protected]>

Extend writable interface to support AD provider

0785db1

This commit adds missing code to extend writable interface to write back user modifications for attributes defined in AD integration domain. Signed-off-by: Francisco Trivino <[email protected]>

f-trivino force-pushed the domains branch from f454a30 to 0785db1 Compare March 23, 2023 11:34

flo-renaud approved these changes Mar 23, 2023

View reviewed changes

f-trivino merged commit 7c4d682 into freeipa:main Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial Domains REST API endpoint #17

Add initial Domains REST API endpoint #17

f-trivino commented Jan 12, 2023

flo-renaud left a comment

flo-renaud commented Feb 17, 2023

flo-renaud left a comment

flo-renaud Feb 17, 2023

f-trivino Mar 15, 2023

flo-renaud Mar 15, 2023

flo-renaud Feb 17, 2023

f-trivino Mar 15, 2023

flo-renaud left a comment

flo-renaud Mar 15, 2023

flo-renaud left a comment

flo-renaud Mar 16, 2023

flo-renaud Mar 16, 2023

f-trivino Mar 22, 2023

f-trivino commented Mar 17, 2023

flo-renaud left a comment

Add initial Domains REST API endpoint #17

Add initial Domains REST API endpoint #17

Conversation

f-trivino commented Jan 12, 2023

flo-renaud left a comment

Choose a reason for hiding this comment

flo-renaud commented Feb 17, 2023

flo-renaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flo-renaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flo-renaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-trivino commented Mar 17, 2023

flo-renaud left a comment

Choose a reason for hiding this comment