-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 On Thu, May 25, 2023 at 10:54:48AM +0000, Ben Grande wrote: > On 23-05-24 14:57:12, Demi Marie Obenour wrote: > > On Wed, May 24, 2023 at 11:53:51AM +0000, Ben Grande wrote: > > > Contrary to what doc/package-contributions says to do a brief > > > description, I prefer a long explanation than having to answer questions > > > in future mails when I could have answered them upfront. > > > > I like this approach :). Thanks for the contribution! > > Thanks for the review. > > > > I saw that Marta Marczykowska-Gorecka is doing the a GUI application > > > called qubes-policy-editor. I can't deny it has a better usability for > > > people not used to terminal applications but it doesn't work for > > > headless dom0, or embedding the policy syntax into fenced code blocks in > > > Markdown or Rst. Because the syntax also can be used in fenced code > > > blocks, people who edit the Qrexec Policy documentation will have the > > > benefit of getting the syntax inline (g:markdown_fenced_languages). In > > > the end, the user will choose what suits him best, I am just providing > > > an alternative. > > > > Nice! This does mean that the syntax file needs to be secure against > > maliciously crafted policy. It looks like there are some dynamically > > generated regular expressions that could be replaced with stridx() and > > string slicing. Otherwise, the code looks to be pretty good: there are > > no calls to execute() or system(), and the code doesn’t open any files > > with paths that are determined by the policy. > > About being secure against a maliciously crafted policy, normally, the > best thing is to open the policy without a modeline. > ```sh > vim -c "setlocal nomodeline ft=qrexecpolicy spell" malicious.policy > ``` > The option 'nomodeline' is set in $VIMRUNTIME/ftplugin/mail.vim with the > following comment: > ```vim > " Don't use modelines in e-mail messages, avoid trojan horses and nasty > " "jokes" (e.g., setting 'textwidth' to 5). > setlocal nomodeline > ``` > > The downside is that the filetype will not be set by the modeline when > using a policy that is not in the expected path (ftdetect). So using > ```qrexecpolicy > # vim: ft=qrexecpolicy > # My custom policy > !compat-4.0 > ``` > wouldn't work if the path to policy is /tmp/custom.policy for example. > > Currently, modeline is not disabled, it uses Vim's or user settings, > which defaults to enabled. > > Can you please point to the dynamically generated regular expression? > My understanding is that dynamically generated regular expressions > requires the execute command: > https://github.com/vim/vim/blob/master/runtime/syntax/yaml.vim#L146
=~ interprets its RHS as a regular expression, so any VimScript that uses =~ with a non-constant RHS needs to ensure that the RHS is not controlled by an attacker. In your case it appears that e.g. subscripting and stridx() would be better. > It has some nice uses cases to avoid code repetition and using variables > to keep regex, but I didn't do that for vim-qrexec. > > The only file read is the current buffer during code completion > autoload/qrexeccomplete.vim - getline(). Which Vim has already read so > it can show you its contents, now it is read to be saved to a variable. > > > > Dependencies > > > - Vim version 8.2: stable version for Dom0 Fedora 32 and Debian 11 > > > > Do later versions work as well? > > Tested now on Fedora 37, Vim 9.0. It worked. > No complaints about spell file compiled on an older version (8.2), and > the spell file worked. Maybe Vim's error E770 appears only when Vim does > breaking changes to the spell file. > > Vim has history of avoiding breaking changes, so I assumed it will > continue working for later versions. But it still makes sense to use a > postinstall to recompile the spell file when Vim is updated. > > > > Syntax > > > ------ > > > The syntax highlighting follows Woju's code[0]. > > > > > > The valid highlighting is nice, but the real benefit of using the syntax > > > file is by making it strict: > > > - Validation for every field > > > - No field can exist without a group > > > - Requires minimum field number to be reached > > > - Shows clearly where the error is > > > - Items of the 2nd field and beyond are always contained and only > > > matched by 'syntax match' option 'nextgroup='. > > > > That is awesome! > > Thanks, this is a thank you letter to the Qubes Team. Also a way to > mitigate easy debugging and avoid breaking Qrexec call because you > couldn't know what was wrong without watching the logs with: > ```sh > sudo journalctl -fu qubes-qrexec-policy-daemon.service > ``` > Which has a bad usability and unnecessary on most cases as this plugin > will prove. > > > > Lint > > > ---- > > > Wrapper around 'makeprg' with qubes-policy-lint for native method. > > > If you prefer a third-party plugin, variable for Dispatch and lint > > > script for ALE is defined. > > > > > > Lint catches cases where the syntax would not be a good way to show the > > > problem. There is no perfect solution anyway, it is not possible to > > > handle runtime bugs. > > > > > > Code completion > > > --------------- > > > When the field has know possible values such as the default arguments, > > > tokens, resolutions, parameters, it is added by default to the > > > completion list. > > > > > > For those same fields, the list is incremented for every line that > > > reaches the minimum number of fields. If a rule has a qube called > > > 'sourcevm' as the source for the rule, the 'sourcevm' will be added to > > > the list of possible values for source completion. > > > > NICE!!! > > On the code completion case, it is dynamically generated, so we might > need to do something here. I don't know the risk of inserting text that > is already on the file to the completion list and then accepted by the > user. What would be a good filtering method? Block characters that are > not allowed by Qrexec? Chars outside of the range: [A-Za-z0-9_+*.=-] > Currently not implemented. I would go with the former. > > > Spell checking > > > -------------- > > > If you set 'spell', you will benefit by using the plugin spell file as a > > > secondary good word list, after your primary language. It helps to > > > reduce the number of false positives spelling errors. > > > > Is this handled automatically? > > No. But I could improve it a little by setting in ftplugin the > following: > ```vim > setlocal spell > setlocal spelllang=en_us > ``` > The only thing I am afraid of is the user not understand how spell works > and starts adding his words to the plugin good words list, which will be > overwritten on updates. User still needs to set his good words list with > 'spellfile'. > > A more long explanation of the current state below. > > The ftplugin appends the qrexec spell file to Vim's option 'spellfile'. > By itself, it is not of much use, it requires the that the user has > already set their spelling configuration: > ```sh > mkdir -p ~/.vim/after/spell > ``` > ```vim > set spell > set spelllang=en_us > set spellfile=~/.vim/after/spell/en.utf-8.add > ``` > > The policy comments are written in English. The Spell is not enabled by > default. I'd rather not set the language spell file for the user. > If the user wants spell checking for comments, he needs to set the above > options. > > Do you think it makes sense force 'spell'? I think so, but it doesn't > work if user doesn't set the 'spelllang'. > Should we force the 'spelllang' to english? If someone writes their > comments in another language, they can also append to the spelllang: > ```vim > # vimrc > # Append Polish to the spelling language of qrexec files: > augroup qrexec > autocmd! > autocmd FileType qrexecpolicy,qrexecpolicyservice,qrexeconfig > \ setlocal spelllang+=pl > augroup END > ``` > > The 'spellfile' is the user good words list. We should not modify it. > But perhaps we can enable 'spell' and set 'spelllang' to US English. I’ll leave this to the other Qubes developers. > > > Questions > > > ========= > > > I have a few questions, if you would be kind to answer them: > > > > > > 1 - Would QubesOS Team package the vimfiles to for Dom0 and DomUs? > > > Only Dom0 can lint, as it requires the qubes-qrexec-dom0 to be > > > installed, but DomUs can greatly benefit from this plugin by using > > > all of the rest it offers, syntax highlighting, code completion, > > > spell check. > > > > Personally I don’t see any reason not to ship them. What would be > > needed to support linting outside of dom0? > > If the parser can be packaged to DomUs, that would make linting policies > from any qube possible. These are the dependencies I mapped: > > qrexec > __init__ > exc > utils > policy.parser > > See the imports at: > https://codeberg.org/ben.grande.b/qubes-tools/src/branch/main/qubes-policy-lint That should definitely be doable. > > > 1.1 - Is VimScript a barrier for inclusion as it greatly decreases the > > > chances of someone reviewing it? Total lines of code excluding > > > comments, empty lines or lines that multi-lines: 957, as of > > > 2023-04-15, first draft of this e-mail. > > > > I am not personally proficient in VimScript, but IIUC syntax files are > > largely declarative, which should help. > > Great. The most complicated is the code completion at > autoload/qrexeccomplete.vim. Yeah, that’s where I saw the dynamically generated regexps. > > > 1.2 - How can I help reviewers? I documented the code via comments and > > > the usage via the help file. Is there anything that I could clarify > > > further? Don't hesitate to ask questions. > > > > > > 2 - There is one binary and only because Vim requires the spell file > > > to be compiled, it is at spell/qrexec.ascii.add.spl. Information on > > > how to generate the file is documented at spell/qrexec.ascii.add, > > > scripting the compilation is possible so you don't depend on me. It > > > may vary on different Vim versions and must be compiled with the > > > same version that is gonna later be used by the users else Vim will > > > throw an error. > > > > Simplest option would be to build it in an postinstall script and use a > > trigger to make sure that it gets updated when the Vim package is > > updated. > > Yes. For reference, see :h E771 > All issues in that section are applicable, E770, E771, E772. > When I tested on Vim 9.0, this issue didn't occur. Good to know! > > > 3 - What is the recommended style for indentation? Tabs? Spaces? > > > Textwidth? I made some assumptions based on the current files > > > shipped by Qubes. Tabs are expanded to spaces and indentation is > > > made as 2 (two) spaces. Textwidth is set at 78 for comments, leaving > > > 2 columns for the sign column or line number. Rules are wrapped, > > > they do not break with the textwidth, but comments break after > > > textwdith for uniformity. Please search the code for > > > 'g:qrexec_recommended_style' to suggest changes. > > > > Qubes OS uses 4 spaces for indentation in C and Python, but I am not > > sure what the standard is for VimScript. > > I meant the policy coding style. For Vim I used 2 spaces, 78 textwidth. > You can see the modeline on each file, which, should be the same. > > -- > Benjamin Grande - -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEdodNnxM2uiJZBxxxsoi1X/+cIsEFAmRvgqkACgkQsoi1X/+c IsGA5w//bCndEID+TBhnfsuxB34Zjg18Ot+yCinw083kr4iubazDzyGsLsGj83ig e3Lm/N68NB4xIKsbnwVL/BDllgo/3owsFzoWEgEZWQRRPXumDRakLW9wNsMsaLTD 6nGWdhIeTEmE+v+xCIllkSHQJcTIj95VuvNdEtp4LjKdF0PFzeyrAVC00SU8k/Pb ArttfJCdtHKsZCj3Uk1K2gLD15JkiqyfIiLGuP1CTtE9NZoVvsKbhdYUuplWJbyi hUOLOZ+PDmZQKUJZLinh1Ev99qplyc/NADvSNHs30VfKnM8FSmhStlleQ9XLhZmL DTw7V5dB4di3+eQLLDzqCZnN/AT2sXzUxF/igEL/o0PDZMCMq6Y2V4WjOvRmAXZf fD5HtNKkWlCm/31f8Sgz+456NzQH2xiF/I32xsPpudKB247HCvMt8P0QtLce6TIa BslflWmhX2pT3wzaPhNqmkGYhOW8N3qxu2XufE28EEeQjcysk/rYYtG9JU7Oh4Gw YS6HPLRGcs+Zo9LtJqMZOqMAx0nwrhk2ta0kXkZqgL4ufJLBHe5i5ycMRReX6jyN HFk/jLnmcu9F2tw58GNEf34NYlnd3FFSHv93TWmoF5HjK8RrSUZSobcQJpTNvdNf BGf2bYa+FC0n+FYBxkitxo4u75+/v/mgHQrdagXBi8BUz4IDdXo= =AKoK -----END PGP SIGNATURE----- -- You received this message because you are subscribed to the Google Groups "qubes-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to qubes-devel+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/qubes-devel/ZG%2BCqSJ13UhBXA1X%40itl-email.
