Modify

Opened 8 years ago

Last modified 10 months ago

#68 review defect

GetComponents non interactive in case of certificate issues.

Reported by: diener@… Owned by: Eric Seidel
Priority: minor Milestone:
Component: GetComponents Version: development version
Keywords: newuser Cc: eric@…

Description

I did a complete new checkout of the EinsteinToolkit on numrel06 using the version of GetComponents available on the EinsteinToolkit web pages and had GetComponents
hang with no errors or warnings when checking out AEIThorns/AEILocalInterp.

Trying the checkout manually I got:

Error validating server certificate for 'https://svn.aei.mpg.de:443':

  • The certificate is not issued by a trusted authority. Use the fingerprint to validate the certificate manually!

Certificate information:

  • Hostname: svn.aei.mpg.de
  • Valid: from Tue, 23 Feb 2010 16:02:12 GMT until Sun, 22 Feb 2015 16:02:12 GMT
  • Issuer: Max-Planck-Gesellschaft, DE
  • Fingerprint: 03:b4:e8:6e:d9:09:e9:93:72:e9:ff:fa:df:e4:2c:6d:1d:2a:e4:66

(R)eject, accept (t)emporarily or accept (p)ermanently? p

After accepting the certificate and restarting the checkout proceeded without problems.

Attachments (4)

cert.patch (2.0 KB) - added by Eric Seidel 6 years ago.
0001-rewrite-run_command-routine-to-capture-stdout-and-st.patch (5.2 KB) - added by Roland Haas 6 years ago.
svn (112 bytes) - added by Roland Haas 6 years ago.
GetComponents_svn_non-interactive (985 bytes) - added by Frank Löffler 6 years ago.
make all svn commands non-interactive

Download all attachments as: .zip

Change History (33)

comment:1 Changed 8 years ago by Eric Seidel

Status: newaccepted

I have seen this once or twice myself, and I'm not sure how to handle it best. For some reason, even with full verbosity, GetComponents will not print out the entire error. The last line is never shown, unless you know to enter 'p', in which case it displays the last line once you hit Enter. I think the problem may be with the fact that the output from svn is piped into perl for parsing, instead of passing control to svn entirely. I suppose I could check for the first part of the error message, and produce the rest myself in Perl.

comment:2 Changed 8 years ago by Erik Schnetter

When one opens a pipe, one can often set what kind of auto-flush is desired. Flushing ensures that the buffers are emptied and all content is sent to the receiver. Typical buffering is block buffering (e.g. 512 bytes), line buffering (each newline empties the buffer) and no buffering. If you don't see the last, partially written line, then you are probably using line buffering, and you should switch to no buffering instead. In C, the function setvbuf can be used to change the buffering kind for a file.

comment:3 in reply to:  2 Changed 8 years ago by Eric Seidel

Replying to eschnett:

When one opens a pipe, one can often set what kind of auto-flush is desired. Flushing ensures that the buffers are emptied and all content is sent to the receiver. Typical buffering is block buffering (e.g. 512 bytes), line buffering (each newline empties the buffer) and no buffering. If you don't see the last, partially written line, then you are probably using line buffering, and you should switch to no buffering instead. In C, the function setvbuf can be used to change the buffering kind for a file.

Yes I was wondering if it might be waiting for a newline to display. I'll check the perl documentation to see how to change the auto-flush condition.

comment:4 Changed 8 years ago by Eric Seidel

Sorry for the long delay on this one... I have no way to control how svn buffers its output, so I had to revise the run_command subroutine to work around it. I'm still adapting some of the calls and output processing, so the changes are living in a new branch 'experimental' on GitHub. Please check it out though and let me know if you still have issues.

comment:5 Changed 8 years ago by Eric Seidel

Resolution: fixed
Status: acceptedclosed

I have merged the experimental changes into the master branch on GitHub, and it is properly passing control to svn to deal with the server certificate issues.

comment:6 Changed 6 years ago by Frank Löffler

Resolution: fixed
Status: closedreopened

comment:7 Changed 6 years ago by Frank Löffler

Milestone: ET_2012_05
Version: ET_2010_06ET_2011_10

comment:8 Changed 6 years ago by Frank Löffler

This has been reported in #830, and might (or not) be releated to #752

comment:9 Changed 6 years ago by Ian Hinder

The servers used in the current ET thornlist do not currently have any certificate problems, so a checkout of the thornlist using GetComponents after deleting the user's .subversion directory is currently working. However, when I manually edit the thornlist to add an https URL with an invalid certificate, GetComponents still hangs when trying to check out that thorn. Even with --verbose, I get only the "Executing: ..." and "In: " lines, then it hangs. Is there a way to increase the verbosity? The help text only mentions using "--verbose". Eric, can you try to reproduce this problem with a server with an invalid certificate?

Is it important that we fix this problem for the upcoming release?

comment:10 Changed 6 years ago by Frank Löffler

Cc: eric@… added
Milestone: ET_2012_05ET_2012_11

It would be nice to have it fixed, but I don't think it should hold up the release.

Eric - can you have a look this week?

Changed 6 years ago by Eric Seidel

Attachment: cert.patch added

comment:11 Changed 6 years ago by Eric Seidel

Hey, sorry for the slow response, just finished my semester earlier in the week :)

Here's a simple patch that seems to work. It requires that GetComponents be run in a bash-compliant shell for the PIPESTATUS command, which is not ideal, but I didn't see a way to get the return code of svn in svn | tee using pure sh.

What do you think?

comment:12 Changed 6 years ago by Frank Löffler

We cannot rely on bash being the shell. What about the following, taken from

http://www.unix.com/shell-programming-scripting/92163-command-does-not-return-exit-status-due-tee.html

       You can use this function to make it easier:

       run() {
         j=1
         while eval "\${pipestatus_$j+:} false"; do
           unset pipestatus_$j
           j=$(($j+1))
         done
         j=1 com= k=1 l=
         for a; do
           if [ "x$a" = 'x|' ]; then
             com="$com { $l "'3>&-
                         echo "pipestatus_'$j'=$?" >&3
                       } 4>&- |'
             j=$(($j+1)) l=
           else
             l="$l \"\$$k\""
           fi
           k=$(($k+1))
         done
         com="$com $l"' 3>&- >&4 4>&-
                    echo "pipestatus_'$j'=$?"'
         exec 4>&1
         eval "$(exec 3>&1; eval "$com")"
         exec 4>&-
         j=1
         while eval "\${pipestatus_$j+:} false"; do
           eval "[ \$pipestatus_$j -eq 0 ]" || return 1
           j=$(($j+1))
         done
         return 0
       }
       
       use it as:
       
       run cmd1 \| cmd2 \| cmd3
       exit codes are in $pipestatus_1, $pipestatus_2, $pipestatus_3

comment:13 Changed 6 years ago by Roland Haas

Why not? :-) The user's shell might not be bash, but what is to stop us from using system("bash -c '$cmd'" (with proper quoting of the single quotes the way simfactory does) when executing commands rather than just system($cmd) the way we do right now? This way we only need bash to be present which I think will always be the case for Cactus (if only because the ExternalLibraries configure.sh scripts assume it).

comment:14 Changed 6 years ago by Frank Löffler

Right, we require bash already elsewhere and it is probably hard to find a system where it is not installed. What we would need now is a patch which does the proper escaping (or use another mechanism like a temporary file).

comment:15 Changed 6 years ago by Roland Haas

to escape, I think:

$cmd =~ s/'/'\\''/g;

will do the trick, since inside single quotes no special characters are considered and the bash will concatenate it like so:

a='ab'\''cd'
echo $a
ab'cd

comment:16 Changed 6 years ago by Frank Löffler

Agreed.

comment:17 Changed 6 years ago by Roland Haas

Alright. I have rewritten the original attemp to avoid having to use tee and not rely on bash. The curernt method uses perl native libraries to open pipes to svn and copy output from it. Works on queenbee and my workstation.

Changed 6 years ago by Roland Haas

Attachment: svn added

comment:18 Changed 6 years ago by Roland Haas

added a small shell script "svn" which outputs a prompt to stderr, reads an answer from stdin, then prints the text to stdout. Then calls the "real" svn.

Useful to test the patches if put into one's PATH before /usr/bin/svn.

comment:19 Changed 6 years ago by Frank Löffler

The patch is rather long. I suggest to apply it only to trunk, to give it some time for tests there and then maybe apply it to the release branch later.

Meanwhile I suggest to apply the patch GetComponents_svn_non-interactive which adds the option '--non-interactive' to two 'svn checkout' commands. This would then abort checkouts with certificate problems without user input, but the error message would at least be visible since GetComponents does not hang anymore.

Changed 6 years ago by Frank Löffler

make all svn commands non-interactive

comment:20 Changed 6 years ago by Frank Löffler

Status: reopenedreview

comment:21 Changed 6 years ago by Roland Haas

Status: reviewreviewed_ok

Agreed.

comment:22 Changed 6 years ago by Frank Löffler

Milestone: ET_2012_11ET_2013_05
Priority: majorminor
Status: reviewed_okreopened
Summary: GetComponents hang due to certificate issues.GetComponents non interactive in case of certificate issues.
Version: ET_2011_10development version

comment:23 Changed 5 years ago by Frank Löffler

The patch GetComponents_svn_non-interactive has been reverted due to problems (#1329).

comment:24 Changed 5 years ago by Frank Löffler

Milestone: ET_2013_05

There are also issues with parallel checkouts and keyboard-interaction, so leaving this open, but removing milestone.

comment:25 Changed 3 years ago by Roland Haas

Status: reopenedreview

0001-rewrite-run_command-routine-to-capture-stdout-and-st.patch would still be nice to have since it allows svn to be used interactively for the initial SSL check in #1801 so that users can approve untrusted servers.

comment:26 Changed 20 months ago by Roland Haas

This has been in review state for 17 months. Does anyone want to review this? Otherwise we should close it and put some warning in GetComponents for new users telling them why downloading the ET fails. I would not like to do that since I think this scares away new users but this change really should not be applied without review.

comment:27 Changed 14 months ago by Ian Hinder

The patch currently doesn't apply. Would it be possible to rebase the patch off master, and maybe put it into a bitbucket pull request, rather than a patch file?

After reading this discussion and trying to understand the issues, it seems there is a lot of complication for something that should be very simple (though I appreciate that life is never that straightforward!). Roland, would it be possible to summarise what is currently understood to be the root cause of the problem, and why the current patch is the best way to address it? Thanks!

comment:28 Changed 14 months ago by Ian Hinder

Keywords: newuser added

comment:29 Changed 10 months ago by Roland Haas

Same state as before. Basically even with verbose set to true GetComponents does not display any prompts from svn about untrusted certificates and thus seems to hang (svn waits for user input).

The proposed patch (updated to a pull request: https://github.com/gridaphobe/CRL/pull/7), uses Perl's IPC code to show stderr and stdout on screen and also captures it in the log file.

The issue with paralell output obscuring svn's question to approved the certificate remains. Not much one can do about this other than trying to touch each svn (or git https for that matter) server once serially to get all questions being asked in serial mode. This happens to be what GetComponents does for svn right now (the svn info calls).

Note all perl modules used by this are part of the core perl modules so are present on all perl installations. Since the original patch worked on QueenBee1 which would be far be the oldest software stack we would care about by now, it should also work on all machines around today.

By now the patch likely only affects what is in the log since the accumulated workarounds seem to by now (almost) let the user interactively accept the cert. The thing blocking this right now is a --non-interactive option passed to the svn info calls at the beginning of the checkout loop.

As a matter of fact if we want something that is a one line change to make the system behave somewhat sane (but not capture output in the log files if --verbose is used) then only (the equivalent) of commit 1106763b91d924142ed5be164aa4a9c45ede7bbd is needed (though using bare backticks is still a bad idea).

Modify Ticket

Change Properties
Set your email in Preferences
Action
as review The owner will remain Eric Seidel.
Next status will be 'reviewed_ok'.
as The resolution will be set.
The resolution will be deleted.
to The owner will be changed from Eric Seidel to the specified user.
to The owner will be changed from Eric Seidel to the specified user.
The owner will be changed from Eric Seidel to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.