Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2019 03 19 update ds create #49

Merged
merged 10 commits into from Apr 4, 2019

Conversation

Firstyear
Copy link
Collaborator

This is a major update to the yast DS integration. This changes from the legacy perl setup, to the new python setup. It also is a large code cleanup and improvements to the process in general.


/home/admin/local_ca/
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really nice howto. BTW what mean that last line with path? You can add some # comments there, so it is still executable.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some fixes on the certificate inclusion for the ds configuration in an other PR, are these included in this PR or are they irrelevant? #48

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those fixes will partially work, and will partially break. The Server-Cert change will break (basically, 389 expects it to be Server-Cert), but the "pkpass" change would work. So that PR will need a refactor I think in light of this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So thinking about it, a possible solution is to ask for the ca, cert and key all as .pem, then we do the pkcs12 generation on your behalf to guarantee it's named correctly?


```
rake test:unit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one trick to also check if package builds is local run of osc with rake osc:build

```
~/.y2log
/var/log/YaST2/y2log
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first one is when you run as user and second when run as root.

README.md Outdated
```

For example logging you can execute YaST with debugging environment variables.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not get much this sentece. If you want to mention that you can enable debug logging with Y2DEBUG=1 I would write it more explicitely.

@@ -9,20 +9,20 @@
# this program; if not, contact SUSE LINUX GmbH.

# Authors: Howard Guo <hguo@suse.com>
# William Brown <wbrown@suse.de>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw we are slightly moving away from this practice as git blame or git log or github contributors already provide this info.


[backend-userroot]
sample_entries = yes
suffix = #{suffix}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just hint, you can use e.g. ERB files https://www.stuartellis.name/articles/erb/ to have it as separate file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too complex for me at the moment, as I don't know that much. I'd rather do a change like that in a follow up.

stdin, stdouterr, result = Open3.popen2e('/usr/sbin/dscreate', '-v', 'from-file', '-n', DS_SETUP_INI_PATH)
stdouterr.readlines.map { |l| append_to_log(l) }

if result.value.exitstatus != 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

above your append to log do only debug logging. So in case of failure. I really suggest here to log to info or warn when customers report a bug, it is useful.

if !ok
Popup.Error(_('Failed to enable TLS! Log output may be found in %s') % [DS_SETUP_LOG_PATH])
raise
Popup.Error(_('Failed to set up new instance! Log output may be found in ~/.y2log or /var/log/YaST/y2log'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is message for user. So I would mention only /var/log/YaST/y2log as in production this module runs as root.

@Firstyear
Copy link
Collaborator Author

Updated based on feedback.

@pzirnik
Copy link

pzirnik commented Mar 29, 2019

I am ok with it and drop my changes, however i do miss following:

  • looks like ldap and kerberos services are not enabled, should not YaST take care of it ?
  • run of kdb5_util requires krb5.conf and kdc.conf in place
  • debug output for kdb5_ldap_util is still active ?
  • the user is not asked for the password to import his pk12 file
  • how is the "pwdfile.txt" created ?
  • should the CA not be installed into Systems CA store and a ldap.conf be created, that commandline ldap tools do work with TLS ? (for example YaST2 Users will try to use TLS if possible and will fail because of no trust)
  • If the cert is required to be named "Server-Cert" either we need to take care to name it this way or let the user know this requirement ?

@Firstyear
Copy link
Collaborator Author

I am ok with it and drop my changes, however i do miss following:

* looks like ldap and kerberos services are not enabled, should not YaST take care of it ?

dscreate does the systemctl enable for 389, I am not aware of what kerberos is doing ...

* run of kdb5_util requires krb5.conf and kdc.conf in place

I'm not really sure about the kerberos parts here, my goal in this PR is to make 389 work correctly.

* debug output for kdb5_ldap_util is still active ?

See above, only interest in this PR is making 389 work. If there are krb issues, we should open new issues.

* the user is not asked for the password to import his pk12 file

No, the current assumption is there is no p12 password (as has been for a long time). There was a seperate pr that allowed requesting the p12 password, and I'd like to integrate that in the future as an option.

* how is the "pwdfile.txt" created ?

dscreate generates it as part of the installation process.

* should the CA not be installed into Systems CA store and a ldap.conf be created, that commandline ldap tools do work with TLS ? (for example YaST2 Users will try to use TLS if possible and will fail because of no trust)

No, that's an "auth client" configuration concern. Not a server install concern.

* If the cert is required to be named "Server-Cert" either we need to take care to name it this way or let the user know this requirement ?

Yes, for the moment the UI now lists that it must be named Server-Cert, however, a current thought is that we should allow the user to submit PEM formatted key/cert, and we generate the p12 for import into certutil.

@Firstyear
Copy link
Collaborator Author

Hey there, @jreidinger I think I have addressed all your comments, do you mind reviewing again?

@Firstyear
Copy link
Collaborator Author

Thanks for the approval! Do you mind merging this (squash + merge if possible).

@jreidinger
Copy link
Member

@Firstyear well, you should be able to do it yourself. If not, I will add you as owner to those repos you maintain.

@Firstyear
Copy link
Collaborator Author

I can't do it myself, that's why I was asking :) if you could add me as owner, that would be great. I should ask my team also who else may need access as there was some offers of assistance within the samba team at suse labs to help with this.

@jreidinger
Copy link
Member

@Firstyear done, you should get invitation which will grant you admin right. So you can add also another contributors.

@Firstyear Firstyear merged commit 90458a6 into yast:master Apr 4, 2019
@Firstyear Firstyear deleted the 2019-03-19-update-ds-create branch April 4, 2019 23:44
@yast-bot
Copy link

yast-bot commented Apr 4, 2019

✔️ Public Jenkins job #6 successfully finished

Firstyear added a commit to Firstyear/yast-auth-server that referenced this pull request Jul 2, 2019
* Update ds installer based on latest python tooling for ds packages, as supported by upstream.
jreidinger pushed a commit that referenced this pull request Jan 30, 2020
* Update ds installer based on latest python tooling for ds packages, as supported by upstream.
@jreidinger jreidinger mentioned this pull request Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants