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

Describe limitations of auth-user-pass more clearly #172

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattock
Copy link
Member

@mattock mattock commented Apr 6, 2022

No description provided.

@@ -68,7 +68,11 @@ configuration.
auth-user-pass up

If ``up`` is present, it must be a file containing username/password on 2
lines. If the password line is missing, OpenVPN will prompt for one.
lines. If the password line is missing, OpenVPN will prompt for one on
stdin. If you use a graphical OpenVPN client such as OpenVPN GUI on Windows,
Copy link
Member

Choose a reason for hiding this comment

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

Could we move the "OpenVPN GUI" part as a separate "NOTE" paragraph? Like this:

*NOTE (Windows)*:
    If you use a graphical OpenVPN client such as OpenVPN GUI on Windows,
    you should either define both the username and password in the file, or rely
    on the GUI caching the credentials; defining just the username in the file
    will not work.

Copy link
Member

Choose a reason for hiding this comment

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

We do similar notes and warning sections other places too, just git grep for NOTE and WARNING to see the formatting.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dsommers done. Looks good now? I did not want to tie this to just Windows as other GUI clients might have the same issue due to same reasons.

stdin.

*NOTE*: If you use a graphical OpenVPN client such as OpenVPN GUI on Windows,
you should either define both the username and password in the file, or rely
Copy link
Member

Choose a reason for hiding this comment

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

The NOTE: should be on a single line, and the text should be indented one more level to be correctly rendered.

The current patch gives:

              If up is present, it must be a file containing username/password
              on 2 lines. If the password line is missing, OpenVPN will prompt
              for one on stdin.

              NOTE: If you use a graphical OpenVPN client such as OpenVPN  GUI
              on Windows, you should either define both the username and pass‐
              word in the file, or rely on the GUI  caching  the  credentials;
              defining just the username in the file will not work.

              If  up  is  omitted, username/password will be prompted from the
              console.

With suggested changes:

              If up is present, it must be a file containing username/password
              on 2 lines. If the password line is missing, OpenVPN will prompt
              for one on stdin.

              NOTE:  If you use a graphical OpenVPN client such as OpenVPN GUI
                     on  Windows,  you  should either define both the username
                     and password in the file, or rely on the GUI caching  the
                     credentials; defining just the username in the file

              If  up  is  omitted, username/password will be prompted from the
              console.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you look at server-options.rst you see the same style I've used with NOTE sections:

  *NOTE*: to *change* an option, ``--push-remove`` can be used to first
  remove the old value, and then add a new ``--push`` option with the new
  value.

  *NOTE 2*: due to implementation details, 'ifconfig' and 'ifconfig-ipv6'
  can only be removed with an exact match on the option (
  :code:`push-remove ifconfig`), no substring matching and no matching on
  the IPv4/IPv6 address argument is possible.

They also render the same way as my version. Do we want to do a major surgery in addition to an implant? 😂

Copy link
Member

Choose a reason for hiding this comment

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

Right, there is some inconsistency there. I think we should move all of these NOTE/WARN/INFO (whatever tags we use) to an indented approach, to hightlight it better.

This also gives us better flexibility when we start adding some CSS styling to the HTML rendering, where the indented variant rendering gives much better abilities the formatting and styling. Essentially it changes from a more plain <p>...</p> for "everything" to a <dl><dt>$HEADER</dt><dd>$CONTENT</dd></dl> style.

Copy link
Member

@ordex ordex Sep 17, 2022

Choose a reason for hiding this comment

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

@dsommers should we keep this patch as is and then reformat all the NOTEs with a follow up?

Copy link
Member

Choose a reason for hiding this comment

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

@mattock would you mind rebasing this patch on master and sending it to the mailing list? Once that's done, we can close this PR. Thanks!

@flichtenheld
Copy link
Member

@mattock Could you maybe take a look at this old PR and submit it to the mailing list? Or would you prefer someone taking it over and finishing it up?

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.

4 participants