WillyXJ / facileManager

A modular suite of web apps built with the sysadmin in mind.
www.facilemanager.com
GNU General Public License v2.0
84 stars 37 forks source link

[FEATURE REQUEST/BUG?] Add placeholder "@" support for domain in template values #593

Open decryptedchaos opened 9 months ago

decryptedchaos commented 9 months ago

Replace everything between stars with current version of your facileManager and module installations:
fM Version : 4.5.0 fMDNS Version : 5.3.3

In raising this issue, I confirm the following (please check boxes, eg [X]):


(BUG | ISSUE) Expected Behavior:

Personally I would call this a bug because this is the intended use of templates but I'm sure others would say its not.. hence it's labeled a "Feature Request"

When creating a template zone the idea is to templatize values so you don't have to reenter them each time..

Now, i wanted to create some txt records namely SPF records which when configured properly include the domain name in key spots.

I had hoped using the @ placeholder in the value would suffice to append the domain to the correct location but it doesn't work this way.

WillyXJ commented 4 months ago

This is now added in fmDNS 6.0.0 and later.

WillyXJ commented 4 months ago

617 reports a use case where @ is necessary without translation. A new solution will need to be implemented.

imyller commented 4 months ago

I'd replace the record value with domain name only if "@" alone is the record value (trimmed), but keep the '@' untouched if it is part of longer string.

gianlucagiacometti commented 4 months ago

I think that would anyway compromise the initial idea of the feature request. The request stated that "i wanted to create some txt records namely SPF records which when configured properly include the domain name in key spots", so the domain name is within a longer string. The problem has no easy and immediate solution in my opinion.

gianlucagiacometti commented 4 months ago

Maybe using a different char as a wildcard in TXT records

imyller commented 4 months ago

I personally would not augment BIND TXT formatting with custom preprosessing or variable expansion, but I can see why such feature request was made.

If such feature is required in fmDNS, I'd implement a full variable expansion rather than simplistic character replacement - as it is prone to a lot of parsing issues. One such case is when TXT record is populated with (mime?) encoded data that might contain any allowed character. Those would fail randomly and unexpectedly.

One option is to introduce an escape character such as $ followed by mandatory brackets {} and leave all others than full ${variable} entities unformatted and as is. Therefore accidental $ would not trigger parameter expansion nor would lone ${ combination. This is not 100% safe as ${<valid chars>} is still possible to occur accidentally but at least it can be mitigated by manual $$ escape by where $${var} is expanded as ${var}.

If $ (or similar) is used as single chacter escape where $$ -> $ and $a -> value there is a higher risk that encoded TXT data contains native $ or $<char> combination and user is required to manually escape all such sequences.

In BIND $ on host label is a reserved character for macros (such as $TTL etc.) but also reserved for special use in $GENERATE macros record value. This makes its choice for this custom preprosessor bit risky as there can be collisions. (https://www.zytrax.com/books/dns/ch8/generate.html)

There is no easy solution that would be completely elegant and perfect, but right choice of escape character followed by bracketed variable name/key should be a possible compromise.

imyller commented 4 months ago

One additional thing to consider and take advice from is how PHP is approaching string variable expansion in future releases:

https://wiki.php.net/rfc/deprecate_dollar_brace_string_interpolation

https://php.watch/versions/8.2/$%7Bvar%7D-string-interpolation-deprecated

They are deprecating ${variable} notation in favor of {$variable}.

There are good reasons for this so maybe when implementing such feature for fmDNS record values this example should be followed.

imyller commented 4 months ago

One way to approach this is to introduce a non-standard record type. For example ETXT (Extended TXT). That would be processed by fmDNS using full variable expansion and/or other preprosessing and converted during buildconf to regular TXT.

This would allow leaving standard TXT record to support full standard charset without risk of collisions.

Edit: URL is already such non-standard "virtual" record type - so there is a precedent.

More complex variant of this is to introduce a flag to fmDNS database to mark if variable expansion is enabled for the record or not. This would allow supporting variables in any record type - not just TXT, but would require UI checkbox to be added in record editor.

gianlucagiacometti commented 4 months ago

I like this approach

imyller commented 4 months ago

I like this approach

Would you prefer adding a new virtual record type or adding a flag to fmDNS database and record editor?

Flag would require changes to db schema, but would allow variables in any record type.

gianlucagiacometti commented 4 months ago

A new virtual record ETXT would provide a simpler and immediate solution to the bug the present implementation of #593 created, but it would only be applicable to TXT records. As you pointed out, the flag would be a global solution, and I think it should be deselected by default. The top solution would include an alert if the record value contained the wildcard char '@' and the flag was selected, to confirm the choice. I prefer the flag solution.

imyller commented 4 months ago

if the record value contained the wildcard char '@'

I'd use PHP str_replace and its built-in variable substition for this. This should be wrapped to single function processing all substitutions and allow only limited set of supported variables provided by fM. Such as {hostname}, {domain}, {tld} etc. If the feature needs expansion it would be contained in this "substitution engine" and be easily expandable.

This way if you enable the record preformatting flag for the record ONLY those supported variables would be substituted by their relevant values during buildconf allowing dynamic values based on the relevant timestamp, domain, server etc.

My opinion is that replacing @ with hostname is quite risky even in these specially flagged records. That CAN be done, but I'd still stick with str_replace approach only.

+

str_replace substitution engine is developed in PHP core and is well tested and "battle hardened". It also brings the aforementioned deprecations and changes in the PHP variable spec going forward. Implementing own substition logic is always riskier than standing on the shoulders of the giants.

gianlucagiacometti commented 4 months ago

Actually, it already uses str_replace (line 1343 of server file class_buildconf.php). I agree that using '@' is risky and with supported variables you may be completely opened to future expansions.

WillyXJ commented 3 months ago

The original implementation has been rolled back in fmDNS 6.0.4 and later.

I am still reviewing how to handle this feature request. Using str_replace() is the easiest, but it's a matter of which string to replace as it cannot be @ alone.

The suggestion provided by @imyller (https://github.com/WillyXJ/facileManager/issues/593#issuecomment-1987201148) is a top choice at this time.