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

Jump to bottom

DNS URI support #134

Closed
wants to merge 3 commits into from
Closed

DNS URI support #134

wants to merge 3 commits into from

Conversation

pspacek
Copy link
Contributor

@pspacek pspacek commented Oct 4, 2016 •
edited by MartinBasti

No description provided.

pvoborni
pvoborni reviewed Oct 4, 2016
View reviewed changes
install/ui/src/freeipa/dns.js
{
name: 'urirecord',
attributes: [
'uri_part_priority',
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about the 'doc' text from API,

'uri_part_priority',

can be replaced with standard field definition, e.g., with:

{
    name: 'uri_part_priority', 
    tooltip: '@mo-param:dnsrecord:uri_part_priority:doc'
}

@MartinBasti MartinBasti self-assigned this Oct 4, 2016
@MartinBasti
Copy link
Contributor

jslint failed please fix it (you can leave pep8 as is, to be consistent with current code)

MartinBasti
MartinBasti suggested changes Oct 6, 2016
View reviewed changes
install/share/dns.ldif
@@ -11,7 +11,7 @@ ipaDNSVersion: 2
aci: (targetattr = "*")(version 3.0; acl "Allow read access"; allow (read,search,compare) groupdn = "ldap:///cn=Read DNS Entries,cn=permissions,cn=pbac,$SUFFIX" or userattr = "parent[0,1].managedby#GROUPDN";)
aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Add DNS entries in a zone";allow (add) userattr = "parent[1].managedby#GROUPDN";)
aci: (target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Remove DNS entries from a zone";allow (delete) userattr = "parent[1].managedby#GROUPDN";)
aci: (targetattr = "a6record || aaaarecord || afsdbrecord || aplrecord || arecord || certrecord || cn || cnamerecord || dhcidrecord || dlvrecord || dnamerecord || dnsclass || dnsttl || dsrecord || hinforecord || hiprecord || idnsallowdynupdate || idnsallowquery || idnsallowsyncptr || idnsallowtransfer || idnsforwarders || idnsforwardpolicy || idnsname || idnssecinlinesigning || idnssoaexpire || idnssoaminimum || idnssoamname || idnssoarefresh || idnssoaretry || idnssoarname || idnssoaserial || idnsupdatepolicy || idnszoneactive || ipseckeyrecord || keyrecord || kxrecord || locrecord || mdrecord || minforecord || mxrecord || naptrrecord || nsecrecord || nsec3paramrecord || nsrecord || nxtrecord || ptrrecord || rprecord || rrsigrecord || sigrecord || spfrecord || srvrecord || sshfprecord || tlsarecord || txtrecord || unknownrecord ")(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Update DNS entries in a zone";allow (write) userattr = "parent[0,1].managedby#GROUPDN";)
aci: (targetattr = "a6record || aaaarecord || afsdbrecord || aplrecord || arecord || certrecord || cn || cnamerecord || dhcidrecord || dlvrecord || dnamerecord || dnsclass || dnsttl || dsrecord || hinforecord || hiprecord || idnsallowdynupdate || idnsallowquery || idnsallowsyncptr || idnsallowtransfer || idnsforwarders || idnsforwardpolicy || idnsname || idnssecinlinesigning || idnssoaexpire || idnssoaminimum || idnssoamname || idnssoarefresh || idnssoaretry || idnssoarname || idnssoaserial || idnsupdatepolicy || idnszoneactive || ipseckeyrecord || keyrecord || kxrecord || locrecord || mdrecord || minforecord || mxrecord || naptrrecord || nsecrecord || nsec3paramrecord || nsrecord || nxtrecord || ptrrecord || rprecord || rrsigrecord || sigrecord || spfrecord || srvrecord || sshfprecord || tlsarecord || txtrecord || urirecord || unknownrecord ")(target = "ldap:///idnsname=*,cn=dns,$SUFFIX")(version 3.0;acl "Update DNS entries in a zone";allow (write) userattr = "parent[0,1].managedby#GROUPDN";)
Copy link
Contributor

Choose a reason for hiding this comment

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

In the same way, you have to update 40-dns.update file, otherwise upgrade will not work (don't forgot to remove original ACI there as well)

ipaserver/plugins/dns.py
@@ -28,6 +28,7 @@

import dns.name
import dns.exception
import dns.rdata
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this import needed?

@MartinBasti
Copy link
Contributor

NACK, please see inline comments

@MartinBasti
Copy link
Contributor

I was able to add an invalid URI record

[root@vm-058-017 ~]# ipa dnsrecord-add test.zone. --uri-rec='0 0 trolo"lo'
Record name: test2
  Record name: test2
  URI record: 0 0 "trolo"lo"

[root@vm-058-017 ~]# dig +short test2.test.zone. URI
[root@vm-058-017 ~]# 

journalctl output
failed to parse RR entry:  resource record DN 'idnsname=test2,idnsname=test.zone.,cn=dns,dc=blabla' data '0 0 "trolo"lo"': extra input text

@pspacek pspacek force-pushed the dns_uri branch 2 times, most recently from 38e3eb8 to 226f897 Compare October 6, 2016 12:27
@pspacek
Copy link
Contributor Author

pspacek commented Oct 6, 2016

I was playing with an idea of automatic escaping but it cannot be done with current record format: There is no way to distinguish alredy escaped text from a text which needs escaping. This totally breaks web UI edit workflow because it reads text from LDAP, fills text field with it and then double-escapes it during save.

For this reason I've given up attempts to automatically escape things, which is no different from e.g. TXT records. Call it consistency if you want ;-)

@pspacek pspacek force-pushed the dns_uri branch 2 times, most recently from 87f4636 to 7068753 Compare October 6, 2016 12:49
@MartinBasti
Copy link
Contributor

I'm so sorry, I didn't noticed earlier, but you forgot to bump API in VERSION

Otherwise LGTM and works for me

@pspacek
Copy link
Contributor Author

pspacek commented Oct 10, 2016

I thought that messing with API numbers in VERSION is not be necessary anymore after we introduced thin client. @jcholast ?

@MartinBasti MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Oct 11, 2016
@MartinBasti
Copy link
Contributor

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/f363dfbeed7aeba51d694a60b29389d94c7bda44
https://fedorahosted.org/freeipa/changeset/bf96b80200c3e8d1795a913db4e0222ea558e609
https://fedorahosted.org/freeipa/changeset/51af7a15981eca753dfbda133cc557d0512f6e95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvoborni pvoborni pvoborni left review comments

@MartinBasti MartinBasti MartinBasti requested changes

Assignees

@MartinBasti MartinBasti

Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Milestone
No milestone
3 participants
@pspacek @MartinBasti @pvoborni